[RFC] uimagefs segmentation fault

[RFC] uimagefs segmentation fault

[Antony Pavlov]

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.

3. don't show absent attributes, e.g.:

@@ -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));
@@ -421,7 +421,7 @@ static int __uimage_open(struct uimagefs_handle *priv)
                goto err_out;
 
        ret = uimagefs_add_os(priv);
-       if (ret)
+       if (ret && ret != -ENODATA)
                goto err_out;
 
        ret = uimagefs_add_time(priv);


I'm not a skilled uImage user so I have no idea which solution is acceptable for the users.

Please comment!

-- 
Best regards,
  Antony Pavlov

Re: [RFC] uimagefs segmentation fault

[Sascha Hauer]

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 |