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 23:03:41 +0800
From:   Zhangjin Wu <falcon@...ylab.org>
To:     falcon@...ylab.org
Cc:     David.Laight@...LAB.COM, arnd@...db.de,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        thomas@...ch.de, w@....eu
Subject: Re: [PATCH v5] tools/nolibc: fix up size inflate regression

> > On Mon, Aug 14, 2023 at 12:27:48PM +0000, David Laight wrote:
> > > From: Willy Tarreau
> > > > Sent: 14 August 2023 13:10
> > > > 
> > > > 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.
> > > 
> > > Actually something like:
> > > 
> > > #define SET_ERRNO(v) (errno = -(long)(v), __typeof__(v)-1)
> > > 
> > > seems to work and allows the errno assignment be removed.
> > > Also works for pointer types (after a different compare).
> > 
> > Yes, that's something we can do (with the parenthesis around
> > __typeof__(v) though).
> >
> 
> Yes, we need the parenthesis, this works:
> 
>     #define SET_ERRNO(v) ( errno = -(long)(v), ((__typeof__(v))-1))
>

Willy, David, We still need "({})":

	#define SET_ERRNO(v) ({ errno = -(long)(v); (__typeof__(v))-1; })

to silence the warning:

	sysroot/x86_64/include/errno.h:13:42: warning: right-hand operand of comma expression has no effect [-Wunused-value]
	   13 | #define SET_ERRNO(v) ( errno = -(long)(v), (__typeof__(v))-1)
	      |                      ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

BR,
Zhangjin

>     #define __is_unsigned_type(v) ((__typeof__(v))(-1) > (__typeof__(v))1)
>     #define __is_syserr(v)        (__is_unsigned_type(v) ? (long)v & ~(-4095UL) : (v + 1 < (__typeof__(v))1))
> 
>     #define __sysret(arg)                                                          \
>     ({                                                                             \
>             __typeof__(arg) __sysret_arg = (arg);                                  \
>             (!__is_syserr(__sysret_arg)) ? __sysret_arg : SET_ERRNO(__sysret_arg); \
>     })
> 
> It looks better now, even for 'void *'.
> 
> '(long)x & ~(-4095UL)' saves another 2+ bytes in some architectures:
> 
>      i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19256
>    x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21740
>     arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 25812
>       arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22856
>      mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22756
>       ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26380
>     ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26756
>   ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27364
>     riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21758
>      s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 21936
> 
> BR,
> Zhangjin