| 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
| ||
|
Message-ID: <272cb38a-c0e3-4e6e-89ce-b503c75c2c33@csgroup.eu>
Date: Tue, 27 Aug 2024 19:14:44 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Vincenzo Frascino <vincenzo.frascino@....com>,
Arnd Bergmann <arnd@...db.de>, "Jason A . Donenfeld" <Jason@...c4.com>
Cc: Theodore Ts'o <tytso@....edu>, Andy Lutomirski <luto@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK
Le 27/08/2024 à 18:05, Vincenzo Frascino a écrit :
> Hi Christophe,
>
> On 27/08/2024 11:49, Christophe Leroy wrote:
>
> ...
>
>
>>>
>>> These are still two headers outside of the vdso/ namespace. For arm64
>>> we had concluded that this is never safe, and any vdso header should
>>> only include other vdso headers so we never pull in anything that
>>> e.g. depends on memory management headers that are in turn broken
>>> for the compat vdso.
>>>
>>> The array_size.h header is really small, so that one could
>>> probably just be moved into the vdso/ namespace. The minmax.h
>>> header is already rather complex, so it may be better to just
>>> open-code the usage of MIN/MAX where needed?
>>
>> It is used at two places only so yes can to that.
>>
>
> Could you please clarify where minmax is needed? I tried to build Jason's master
> tree for x86, commenting the header and it seems building fine. I might be
> missing something.
Without it:
VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:11,
from <command-line>:
./arch/powerpc/include/asm/vdso/getrandom.h: In function
'__arch_get_vdso_rng_data':
./arch/powerpc/include/asm/vdso/getrandom.h:46:9: error: implicit
declaration of function 'BUILD_BUG' [-Werror=implicit-function-declaration]
46 | BUILD_BUG();
| ^~~~~~~~~
./arch/powerpc/include/asm/vdso/getrandom.h:47:1: error: no return
statement in function returning non-void [-Werror=return-type]
47 | }
| ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c: In function
'__cvdso_getrandom_data':
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:76:23: error: implicit
declaration of function 'min_t' [-Werror=implicit-function-declaration]
76 | ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* =
MAX_RW_COUNT */, len);
| ^~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:76:29: error: expected
expression before 'size_t'
76 | ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* =
MAX_RW_COUNT */, len);
| ^~~~~~
In file included from ./include/linux/array_size.h:5,
from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:6:
./include/linux/compiler.h:243:33: error: implicit declaration of
function 'BUILD_BUG_ON_ZERO' [-Werror=implicit-function-declaration]
243 | #define __must_be_array(a)
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
| ^~~~~~~~~~~~~~~~~
./include/linux/array_size.h:11:59: note: in expansion of macro
'__must_be_array'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
|
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:40: note: in
expansion of macro 'ARRAY_SIZE'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:196:27: error: expected
expression before 'size_t'
196 | batch_len = min_t(size_t, sizeof(state->batch) -
state->pos, len);
| ^~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:247:9: error: implicit
declaration of function 'BUILD_BUG_ON'
[-Werror=implicit-function-declaration]
247 | BUILD_BUG_ON(sizeof(state->batch_key) %
CHACHA_BLOCK_SIZE != 0);
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:93:
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2
>
>> Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
>> only once
>>
>
> I have a similar issue to figure out why linux/array_size.h and
> uapi/linux/random.h are needed. It seems that I can build the object without
> them. Could you please explain?
Without linux/array_size.h:
VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from <command-line>:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c: In function
'__cvdso_getrandom_data':
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:40: error: implicit
declaration of function 'ARRAY_SIZE' [-Werror=implicit-function-declaration]
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:93:
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2
Without uapi/linux/random.h:
VDSO32C arch/powerpc/kernel/vdso/vgetrandom-32.o
In file included from <command-line>:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c: In function
'__cvdso_getrandom_data':
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:86:23: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
86 | params->size_of_opaque_state = sizeof(*state);
| ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:87:23: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
87 | params->mmap_prot = PROT_READ | PROT_WRITE;
| ^~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:88:23: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
88 | params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
| ^~
In file included from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:6:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~
./include/linux/array_size.h:11:33: note: in definition of macro
'ARRAY_SIZE'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
| ^~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~
./include/linux/array_size.h:11:48: note: in definition of macro
'ARRAY_SIZE'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
| ^~~
In file included from ./include/linux/minmax.h:5,
from /home/chleroy/linux-powerpc/lib/vdso/getrandom.c:7:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct {
int:(-!!(e)); })))
| ^
./include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
243 | #define __must_be_array(a)
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
| ^~~~~~~~~~~
./include/linux/array_size.h:11:59: note: in expansion of macro
'__must_be_array'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
|
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:40: note: in
expansion of macro 'ARRAY_SIZE'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:57: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct {
int:(-!!(e)); })))
| ^
./include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
243 | #define __must_be_array(a)
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
| ^~~~~~~~~~~
./include/linux/array_size.h:11:59: note: in expansion of macro
'__must_be_array'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
|
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:40: note: in
expansion of macro 'ARRAY_SIZE'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~~~~~~~~~
./include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width
not an integer constant
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct {
int:(-!!(e)); })))
| ^
./include/linux/compiler.h:243:33: note: in expansion of macro
'BUILD_BUG_ON_ZERO'
243 | #define __must_be_array(a)
BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
| ^~~~~~~~~~~~~~~~~
./include/linux/array_size.h:11:59: note: in expansion of macro
'__must_be_array'
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
|
^~~~~~~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:89:40: note: in
expansion of macro 'ARRAY_SIZE'
89 | for (size_t i = 0; i <
ARRAY_SIZE(params->reserved); ++i)
| ^~~~~~~~~~
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:90:31: error: invalid
use of undefined type 'struct vgetrandom_opaque_params'
90 | params->reserved[i] = 0;
| ^~
In file included from ./include/linux/array_size.h:5:
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:99:32: error:
'GRND_NONBLOCK' undeclared (first use in this function); did you mean
'MAP_NONBLOCK'?
99 | if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM |
GRND_INSECURE)))
| ^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
77 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:99:32: note: each
undeclared identifier is reported only once for each function it appears in
99 | if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM |
GRND_INSECURE)))
| ^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
77 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:99:48: error:
'GRND_RANDOM' undeclared (first use in this function)
99 | if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM |
GRND_INSECURE)))
| ^~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
77 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
/home/chleroy/linux-powerpc/lib/vdso/getrandom.c:99:62: error:
'GRND_INSECURE' undeclared (first use in this function)
99 | if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM |
GRND_INSECURE)))
|
^~~~~~~~~~~~~
./include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
77 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
make[2]: *** [arch/powerpc/kernel/vdso/Makefile:93:
arch/powerpc/kernel/vdso/vgetrandom-32.o] Error 1
make[1]: *** [arch/powerpc/Makefile:388: vdso_prepare] Error 2
make: *** [Makefile:224: __sub-make] Error 2
>
> In general, the philosophy adopted to split the headers is to extract the set of
> functions used by vDSOs from the linux headers and place them in the vdso headers.
> Consequently the linux header includes to vdso one. E.g.: linux/time64.h
> includes vdso/time64.h.
>
> IMHO we should follow the same approach, if at all for consistency, unless there
> is a valid reason.
Indeed I started with something that didn't build and I did the simplest
I could to get it build. I agree with you at the end that would be a
best, can be done in follow-up patches I guess.
>
> ...
>
>>>
>>> Including uapi/linux/mman.h may still be problematic on
>>> some architectures if they change it in a way that is
>>> incompatible with compat vdso, but at least that can't
>>> accidentally rely on CONFIG_64BIT or something else that
>>> would be wrong there.
>>
>> Yes that one is tricky. Because uapi/linux/mman.h includes asm/mman.h with the
>> intention to include uapi/asm/mman.h but when built from the kernel in reality
>> you get arch/powerpc/include/asm/mman.h and I had to add some ifdefery to
>> kick-out kernel oddities it contains that pull additional kernel headers.
>>
>> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
>> index 17a77d47ed6d..42a51a993d94 100644
>> --- a/arch/powerpc/include/asm/mman.h
>> +++ b/arch/powerpc/include/asm/mman.h
>> @@ -6,7 +6,7 @@
>>
>> #include <uapi/asm/mman.h>
>>
>> -#ifdef CONFIG_PPC64
>> +#if defined(CONFIG_PPC64) && !defined(BUILD_VDSO)
>>
>> #include <asm/cputable.h>
>> #include <linux/mm.h>
>>
>
> I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.
Fully agree.
>
> I am attaching a patch to provide my view on how to minimize the headers
> included and use only the vdso/ namespace. Please, before using the code,
> consider that I conducted very limited testing.
>
> Note: It should apply clean on Jason's tree.
>
> Let me know your thoughts.
>
>>
>> Christophe
>
Powered by blists - more mailing lists