linux-kernel - Re: [PATCH v5] tools/nolibc: fix up size inflate regression

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]

Date:   Mon, 14 Aug 2023 14:09:41 +0200
From:   Willy Tarreau <w@....eu>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     "'Zhangjin Wu'" <falcon@...ylab.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "thomas@...ch.de" <thomas@...ch.de>
Subject: Re: [PATCH v5] tools/nolibc: fix up size inflate regression

Hi David,

On Mon, Aug 14, 2023 at 11:15:51AM +0000, David Laight wrote:
> From: Zhangjin Wu
> > Sent: 14 August 2023 11:42
> ...
> > [...]
> > > > > Sure it's not pretty, and I'd rather just go back to SET_ERRNO() to be
> > > > > honest, because we're there just because of the temptation to remove
> > > > > lines that were not causing any difficulties :-/
> > > > >
> > > > > I think we can do something in-between and deal only with signed returns,
> > > > > and explicitly place the test for MAX_ERRNO on the two unsigned ones
> > > > > (brk and mmap). It should look approximately like this:
> > > > >
> > > > >  #define __sysret(arg)                                                \
> > > > >  ({                                                                   \
> > > > >  	__typeof__(arg) __sysret_arg = (arg);                           \
> > > > >  	(__sysret_arg < 0) ? ({           /* error ? */                 \
> > > > >  		SET_ERRNO(-__sysret_arg); /* yes: errno != -ret */      \
> > > > >  		((__typeof__(arg)) -1);   /*      return -1 */          \
> 
> I'm pretty sure you don't need the explicit cast.
> (It would be needed for a pointer type.)
> Can you use __arg < ? SET_ERRNO(-__arg), -1 : __arg
> 
> Thinking, maybe it should be:
> 
> #define __sysret(syscall_fn_args)
> ({
> 	__typeof__(syscall_fn_args) __rval = syscall_fn_args;
> 	__rval >= 0 ? __rval : SET_ERRNO(-__rval), -1;
> })

Yeah almost, since arg is necessarily signed in this version, it's
just that I manually edited the previous macro in the mail and limited
the amount of changes to what was necessary. It's just that SET_ERRNO
only is an instruction, not an expression:

   #define SET_ERRNO(v) do { errno = (v); } while (0)

Thus the return value doesn't even pass through it. That's why it was
so much simpler before. The rationale behind this was to bring the
ability to completely drop errno for programs where you didn't care
about it. It's particularly interesting when you don't need any other
data either as the program gets strunk from a complete section.

> Since, IIRC, the usage is return __sysret(sycall_fn(args));

> I'm not sure how public SET_ERRO() is.

For now it is entirely, though it's not supposed to. Thomas and I
have been discussing about renaming some internal-use macros and
functions to avoid needlessly exposing them by accident to the
application. These one definitely qualifies.

> But it could include the negate have the value of -1 cast to its argument type?
> I think:
> 	error = -(int)(long)(arg + 0u);
> will avoid any sign extension - the (int) might not even be needed.

So with a signed (int/long) input and errno as int, I don't think
we can have any case where there's any such extension anyway. In
any case we're either copying the int as-is or truncating it.

Regards,
Willy