[PATCH 1/1] lmb: do not panic in lmb_print_region_flags

[Sughosh Ganu]

On Sat, 2 Nov 2024 at 12:02, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> Commit c3cf0dc64f1c ("lmb: add a check to prevent memory overrun")
> addressed a possible buffer overrun using assert_noisy().
>
> Resetting via panic() in lmb_print_region() while allowing invalid
> lmb flags elsewhere is not reasonable.
>
> Instead of panicking print a message indicating the problem.

I had initially considered only printing an error message in case of
an invalid flag, but my thinking was that something like an invalid
flag is a serious issue with a subsystem like lmb. But otoh, your
argument is correct that we don't take the same action elsewhere in
the module.

Acked-by: Sughosh Ganu <sughosh.ganu at linaro.org>

-sughosh

>
> fls() returns an int. Using a u64 for bitpos does not match.
> Use int instead.
>
> fls() takes an int as argument. Using 1ull << bitpos generates a u64.
> Use 1u << bitpos instead.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>  lib/lmb.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 96a055f951e..00c8888936a 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -72,16 +72,22 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
>
>  static void lmb_print_region_flags(enum lmb_flags flags)
>  {
> -       u64 bitpos;
>         const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" };
> +       unsigned int pflags = flags &
> +                             (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY);
> +
> +       if (flags != pflags) {
> +               printf("invalid %#x\n", flags);
> +               return;
> +       }
>
>         do {
> -               bitpos = flags ? fls(flags) - 1 : 0;
> -               assert_noisy(bitpos < ARRAY_SIZE(flag_str));
> +               int bitpos = pflags ? fls(pflags) - 1 : 0;
> +
>                 printf("%s", flag_str[bitpos]);
> -               flags &= ~(1ull << bitpos);
> -               puts(flags ? ", " : "\n");
> -       } while (flags);
> +               pflags &= ~(1u << bitpos);
> +               puts(pflags ? ", " : "\n");
> +       } while (pflags);
>  }
>
>  static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name)
> --
> 2.45.2
>