[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] mount: support the sizelimit and offset option for loop devices
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2020-09-30 22:33:19
Message-ID: CAK1hOcN9Kd48kXdtL2ERuTqL2whRAh6=pEZtUSwccqQYBDgE8A () mail ! gmail ! com
[Download RAW message or body]

Apologies for an extremely late reply!


On Wed, Jul 29, 2020 at 10:44 AM Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> Starting with linux kernel v5.4 squashfs has a more strict parameter
> checking implemented. Unlike util-linux mount, busybox never supported
> the sizelimit option but simply forwards it to the kernel.
> Since v5.4 mounting will fail with
>
>     squashfs: Unknown parameter 'sizelimit'
>
> Support the sizelimit parameter by setting it in the LOOP_SET_STATUS64
> structure before handing it to the kernel.
>
> While at it also add support for the offset option, which currently will
> always be set to 0.
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> +                       /* parse options */
> +                       if (filteropts)
> +                               for (opt = strtok(filteropts, ","); opt; opt = strtok(NULL, ",")) {
> +                                       char *opteq = strchr(opt, '=');
> +                                       int len = strlen(opt);
> +                                       char *match;
> +
> +                                       if (opteq) {
> +                                               int idx;
> +                                               static const char options[] ALIGN1 =
> +                                                       /* 0 */ "offset\0"
> +                                                       /* 1 */ "sizelimit\0";
> +
> +                                               *opteq++ = '\0';

This will destroy any non-matching options, no?
(By truncating them at the first '=' - you never restore it)

> +                                               idx = index_in_strings(options, opt);
> +                                               switch (idx) {
> +                                               case 0: // "offset"
> +                                                       offset = strtoull(opteq, 0, 0);

No error checking?

> +                                                       match = strstr(filteropts, opt);

What are you searching for here? "Where is OPT string in FILTEROPTS?"
Obviously, it is at OPT pointer - it points into FILTEROPTS string.
Or worse yet, your FILTEROPTS has a duplicate earlier on, like
"offset,offset=123"
and now you will perform this surgery:

> +                                                       *match = '\0';
> +                                                       strcat(filteropts, match+len);

on the *wrong part of the string* (you would delete "offset," not
"...,offset=123" part).
Also, you need to use len + 1, no?

These three lines should be replaced with just

                         strcpy(opt, opt + len + 1);

...but there is another bug: you are operating on strtok'ed string.
If you edit it like this, subsequent calls of strtok can fail.


I'm committing a modified patch. Please try current git.
Thank you.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]