linux-kernel - Re: [PATCH 4/6] tools/nolibc: fix integer overflow in i{64,}toa

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: <73b72524-95d2-44ef-8176-67c74c37cc36@t-8ch.de>
Date: Sat, 19 Apr 2025 12:26:01 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Willy Tarreau <w@....eu>
Cc: "Paul E. McKenney" <paulmck@...nel.org>, Shuah Khan <shuah@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 4/6] tools/nolibc: fix integer overflow in i{64,}toa_r()
 and

On 2025-04-19 11:40:08+0200, Willy Tarreau wrote:
> On Wed, Apr 16, 2025 at 08:40:19PM +0200, Thomas Weißschuh wrote:
> > In twos complement the most negative number can not be negated.
> 
> well, if we're picky, that's not really an int overflow since it's only
> used as an unsigned in the end, so -2^(N-1) == 2^(N-1) in twos complement.

It is used as unsigned, but at the point of the cast it's still signed.
>From ccpreference:

The unary minus invokes undefined behavior due to signed integer
overflow when applied to INT_MIN, LONG_MIN, or LLONG_MIN, on typical
(2's complement) platforms.

https://en.cppreference.com/w/c/language/operator_arithmetic

> 
> > @@ -271,16 +271,12 @@ int utoa_r(unsigned long in, char *buffer)
> >  static __attribute__((unused))
> >  int itoa_r(long in, char *buffer)
> >  {
> > -	char *ptr = buffer;
> > -	int len = 0;
> > -
> >  	if (in < 0) {
> > -		in = -in;
> > -		*(ptr++) = '-';
> > -		len++;
> > +		*(buffer++) = '-';
> > +		return 1 + utoa_r(-(unsigned long)in, buffer);
> >  	}
> > -	len += utoa_r(in, ptr);
> > -	return len;
> > +
> > +	return utoa_r(in, buffer);
> >  }
> 
> At -Os it's OK but at -O2 it inflates it a little bit and at -O3 it
> significantly inflates the function (175 -> 320 bytes) due to the two
> calls that get inlined. Have you tried to check if ubsan is happy
> with just this?
> 
> -		in = -in;
> +		in = -(unsigned long)in;

That seems to work. Let's use it.

Thanks!

> Otherwise this variant doesn't inflate it for me and keeps the spirit
> of the original one (i.e. single call):
> 
>   int itoa_r3(long in, char *buffer)
>   {
>         unsigned long uin = in;
>         int len = 0;
> 
>         if ((long)uin < 0) {
>                 uin = -uin;
>                 *(buffer++) = '-';
>                 len++;
>         }
>         len += utoa_r(uin, buffer);
>         return len;
>   }
> 
> I'm also seeing a way to make it even smaller than the original by
> changing utoa_r() so that it takes the original buffer in a 3rd
> argument and emits the difference at the end as the length. This
> allows to always perform a tail call, though I'm not sure we really
> need it. 

Thomas