Re: [RFC] uimagefs segmentation fault

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Antony,

On Thu, Jan 04, 2024 at 01:04:04AM +0300, Antony Pavlov wrote:
> Hi!
> 
> I have prepared a linux uImage file for testing my uImage-related patches.
> 
> barebox$ dumpimage -l ./defaultenv/defaultenv-2-base/uImage
> Image Name:   Linux-6.7.0-rc4-00003-ge185a416f
> Created:      Wed Jan  3 15:28:27 2024
> Image Type:   ARM Linux Kernel Image (uncompressed)
> Data Size:    6131616 Bytes = 5987.91 KiB = 5.85 MiB
> Load Address: 00000000
> Entry Point:  00000000
> 
> I tryed to mount my uImage under mainline master sandbox barebox:
> 
> barebox@Sandbox:/ mount -t uimagefs env/uImage /mnt/
> Segmentation fault
> 
> The problem is in the uimagefs_add_str(). if s == NULL then strlen() fails:
> 
> static int uimagefs_add_str(struct uimagefs_handle *priv, enum uimagefs_type type,
>                                 char *s)
> {
>         struct uimagefs_handle_data *d;
> 
>         d = xzalloc(sizeof(*d));
>         d->type = type;
>         d->name = xstrdup(uimagefs_type_to_str(type));
>         d->data = s;
>         d->size = strlen(s); <<<<<<<<<<<<<<<<<<<<<<<<<<< s == NULL!
> 
>         list_add_tail(&d->list, &priv->list);
> 
>         return 0;
> }
> 
> 
> It looks like my uImage lacks some attributes, e.g. "os".
> 
> So I can propose several solutions for the problem:
> 
> 1. don't call strlen(s) if s == NULL:
> 
> @@ -226,7 +226,7 @@ static int uimagefs_add_str(struct uimagefs_handle *priv, enum uimagefs_type typ
>         d->type = type;
>         d->name = xstrdup(uimagefs_type_to_str(type));
>         d->data = s;
> -       d->size = strlen(s);
> +       d->size = (s != NULL) ? strlen(s) : 0;
> 
>         list_add_tail(&d->list, &priv->list);
> 
> So we will have empty files (len == 0) for absent attributes.
> 
> 2. regard the s == NULL situation as error:
> 
> @@ -222,6 +222,9 @@ static int uimagefs_add_str(struct uimagefs_handle *priv, enum uimagefs_type typ
>  {
>         struct uimagefs_handle_data *d;
>  
> +       if (!s)
> +               return -ENODATA;
> +
>         d = xzalloc(sizeof(*d));
>         d->type = type;
>         d->name = xstrdup(uimagefs_type_to_str(type));
> 
> So we can't mount uImages without required attributes.

I don't see a point in refusing to mount uImages without these
properties, they might not be required and the user should decide
if they are needed or not.

I don't have any preference for 1. or 3. though, so unless anybody
else has a good reason for one or the other go and roll a dice ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |