Re: [linux-next:master] [vsnprintf] 8d4826cc8a: BUG:KASAN:global-out-of-bounds_in_number

[Linus Torvalds]

On Sun, 12 Jan 2025 at 22:11, kernel test robot <oliver.sang@xxxxxxxxx> wrote:
>
> [ 53.288261][ T1140] BUG: KASAN: global-out-of-bounds in number (lib/vsprintf.c:494)
> [   53.295106][ T1140] Read of size 1 at addr ffffffff843035c8 by task ln/1140

Bah. That's this:

    494                         tmp[i++] = (hex_asc_upper[((unsigned
char)num) & mask] | locase);

and the one-byte read is that

    hex_asc_upper[((unsigned char)num) & mask]

access.

And 'mask' is supposed to be either 7 or 15 (for base 8 or 16
respectively), but it turns out that this all comes from (I think)

            pr_info("enabling port %d (%pISpcs)\n",
                    le16_to_cpu(nport->disc_addr.portid),
                    (struct sockaddr *)&port->addr);

in nvmet_rdma_add_port(), and that commit 8d4826cc8a8a ("vsnprintf:
collapse the number format state into one single state") doesn't set
the number base for non-numeric formats (like the 'p').

Or rather, it sets the base to zero.

The '%pISpcs' causes the call chain to be pointer() ->
ip_addr_string() -> ip4_addr_string_sa() -> number(), and there the
number base being zero confuses things terminally.

And then when the 'p' logic calls 'number()' with  zero base then
confuses number printing and the 'mask' logic.

Most other number() callers - for example special_hex_number - will
set the base explicitly, but several of the more specialized pointer
things seem to just pass in the 'spec' from the original pointer
format, and expects the "base" of a pointer spec to be 10.

Which wasn't very obvious, butused to be the default, and that commit
broke that.

Does this trivial one-liner (sorry, whitespace-damaged) fix it?

  --- a/lib/vsprintf.c
  +++ b/lib/vsprintf.c
  @@ -2682,7 +2682,7 @@ struct fmt format_decode(struct fmt fmt,
struct printf_spec *spec)
                p = lookup_state + *fmt.str;
        }
        if (p->state) {
  -             spec->base = p->base;
  +             if (p->base) spec->base = p->base;
                spec->flags |= p->flags_or_double_size;
                fmt.state = p->state;
                fmt.str++;

so that the format decoding will only set the spec base to something
else than 10 if the format actually has a base (i.e. is a
FORMAT_STATE_NUM).

                Linus