linux-kernel - Re: [PATCH] vsprintf: simplify number handling

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <871py5gxh4.fsf@prevas.dk>
Date: Wed, 18 Dec 2024 10:19:03 +0100
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>,  Daniel Borkmann
 <daniel@...earbox.net>,  Florent Revest <revest@...gle.com>,  Steven
 Rostedt <rostedt@...dmis.org>,  LKML <linux-kernel@...r.kernel.org>,  bpf
 <bpf@...r.kernel.org>
Subject: Re: [PATCH] vsprintf: simplify number handling

On Tue, Dec 17 2024, Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> Instead of dealing with all the different special types (size_t,
> unsigned char, ptrdiff_t..) just deal with the size of the integer type
> and the sign.
>
> This avoids a lot of unnecessary case statements, and the games we play
> with the value of the 'SIGN' flags value
>
> Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> ---
>
> NOTE! Only very lightly tested.  Also meant to be purely preparatory. 
> It might be broken. 
>
> I started doing this in the hope that the vsnprintf() core code could
> maybe be further cleaned up enough that it would actually be something
> we could use more generally and export to other users that want to
> basically do the printk format handling. 
>
> The code is *not* there yet, though. Small steps.
>

> +/* Turn a 1/2/4-byte value into a 64-bit one with sign handling */
> +static unsigned long long get_num(unsigned int val, struct printf_spec spec)
> +{
> +	unsigned int shift = 32 - spec.type*8;
> +
> +	val <<= shift;
> +	if (!(spec.flags & SIGN))
> +		return val >> shift;
> +	return (int)val >> shift;
> +}
> +

I think this needs to be a little more explicit that it's not just about
handling sign extension, but also truncation to the desired
width. Otherwise somebody will wonder why this isn't

  if (!(spec.flags & SIGN))
    return val;
  return (int)(val << shift) >> shift;

[with the latter line duplicating our sign_extend32 helper].

The rest looks good.

Rasmus