linux-kernel - Re: [PATCH] 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]

Message-ID: <20230806134348.GA19145@1wt.eu>
Date:   Sun, 6 Aug 2023 15:43:48 +0200
From:   Willy Tarreau <w@....eu>
To:     Zhangjin Wu <falcon@...ylab.org>
Cc:     arnd@...db.de, david.laight@...lab.com,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        thomas@...ch.de
Subject: Re: [PATCH] tools/nolibc: fix up size inflate regression

On Sun, Aug 06, 2023 at 09:19:21PM +0800, Zhangjin Wu wrote:
> As reported and suggested by Willy, the inline __sysret() helper
> introduces three types of conversions and increases the size:

Thanks Zhangjin. A few things:

> --- a/tools/include/nolibc/compiler.h
> +++ b/tools/include/nolibc/compiler.h
> @@ -22,4 +22,13 @@
>  #  define __no_stack_protector __attribute__((__optimize__("-fno-stack-protector")))
>  #endif /* defined(__has_attribute) */
>  
> +/*
> + * from include/linux/compiler.h
> + *
> + * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
> + * bool and also pointer types.
> + */
> +#define is_signed_type(type)   (((type)(-1)) < (type)1)
> +#define is_unsigned_type(type) (!is_signed_type(type))

These names may conflict with application's local definitions. And since
there's a single call place we should probably just inline it instead.

> +#define __sysret(arg)                                                           \
> +({                                                                              \
> +	__typeof__(arg) __sysret_arg = (arg);                                   \
> +	if (is_signed_type(__typeof__(arg))) {                                  \
> +		if (__sysret_arg < 0) {                                         \
> +			SET_ERRNO(-(int)__sysret_arg);                          \
> +			__sysret_arg = -1L;                                     \
> +		}                                                               \
> +	} else {                                                                \
> +		if ((unsigned long)__sysret_arg >= (unsigned long)-MAX_ERRNO) { \
> +			SET_ERRNO(-(int)__sysret_arg);                          \
> +			__sysret_arg = -1L;                                     \
> +		}                                                               \
> +	}                                                                       \
> +	__sysret_arg;                                                           \
> +})

I also found during my tests that this one can return a build error if
the arg has "const" in its type, due to the error assignment. We need
to think about reworking it as a ternary evaluation, it will be more
reliable even if less readable. But let's not change this now, I'm on
the changelog already.

>  /* Functions in this file only describe syscalls. They're declared static so
>   * that the compiler usually decides to inline them while still being allowed
> @@ -94,7 +101,7 @@ void *sbrk(intptr_t inc)
>  	if (ret && sys_brk(ret + inc) == ret + inc)
>  		return ret + inc;
>  
> -	return (void *)__sysret(-ENOMEM);
> +	return (void *)__sysret((unsigned long)-ENOMEM);

I noticed that one as well during my tests, but if we're purist, we're
supposed to use (void*) for the cast and not (unsigned long), and it
should allow to get rid of the outer cast.

Regards,
Willy