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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z4UUnsL85EN/f0BK@xsang-OptiPlex-9020>
Date: Mon, 13 Jan 2025 21:26:54 +0800
From: Oliver Sang <oliver.sang@...el.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
CC: <oe-lkp@...ts.linux.dev>, <lkp@...el.com>, <linux-kernel@...r.kernel.org>,
	<oliver.sang@...el.com>
Subject: Re: [linux-next:master] [vsnprintf] 8d4826cc8a:
 BUG:KASAN:global-out-of-bounds_in_number

hi, Linus,

On Sun, Jan 12, 2025 at 11:07:02PM -0800, Linus Torvalds wrote:
> On Sun, 12 Jan 2025 at 22:11, kernel test robot <oliver.sang@...el.com> 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?

yes, below diff fixes the issue we reported. thanks

Tested-by: kernel test robot <oliver.sang@...el.com>

> 
>   --- 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