| Message ID | 20241102063226.39388-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] lmb: do not panic in lmb_print_region_flags | expand |
On Sat, 2 Nov 2024 at 12:02, Heinrich Schuchardt <heinrich.schuchardt@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@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@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 >
On Sat, 02 Nov 2024 07:32:26 +0100, Heinrich Schuchardt 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. > > [...] Applied to u-boot/master, thanks!
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)
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. 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@canonical.com> --- lib/lmb.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)