Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Sat, Oct 08, 2022 at 09:53:33PM +, David Laight wrote:
> From: Jason A. Donenfeld
> > Sent: 07 October 2022 18:56
> ...
> > > Given these kinds of less mechanical changes, it may make sense to split
> > > these from the "trivial" conversions in a treewide patch. The chance of
> > > needing a revert from the simple 1:1 conversions is much lower than the
> > > need to revert by-hand changes.
> > >
> > > The Cocci script I suggested in my v1 review gets 80% of the first
> > > patch, for example.
> >
> > I'll split things up into a mechanical step and a non-mechanical step.
> > Good idea.
>
> I'd also do something about the 'get_random_int() & 3' cases.
> (ie remainder by 2^n-1)
> These can be converted to 'get_random_u8() & 3' (etc).
> So they only need one random byte (not 4) and no multiply.
>
> Possibly something based on (the quickly typed, and not C):
> #define get_random_below(val) [
> if (builtin_constant(val))
> BUILD_BUG_ON(!val || val > 0x1ull)
> if (!(val & (val - 1)) {
> if (val <= 0x100)
> return get_random_u8() & (val - 1);
> if (val <= 0x1)
> return get_random_u16() & (val - 1);
> return get_random_u32() & (val - 1);
> }
> }
> BUILD_BUG_ON(sizeof (val) > 4);
> return ((u64)get_random_u32() * val) >> 32;
This is already how the prandom_u32_max() implementation works, as
suggested in the cover letter. The multiplication by constants in it
reduces to bit shifts and you already get all the manual masking
possible.
> get_random_below() is a much better name than prandom_u32_max().
Yes, but that name is reserved for when I succeed at making a function
that bounds with a uniform distribution. prandom_u32_max()'s
distribution is non-uniform since it doesn't do rejection sampling. Work
in progress is on
https://git.zx2c4.com/linux-rng/commit/?h=jd/get_random_u32_below .
But out of common respect for this already huge thread with a massive
CC list, if you want to bikeshed my WIP stuff, please start a new thread
for that and not bog this one down. IOW, no need to reply here directly.
That'd annoy me.
Jason
RE: [PATCH v3 3/5] treewide: use get_random_u32() when possible
From: Jason A. Donenfeld
> Sent: 07 October 2022 18:56
...
> > Given these kinds of less mechanical changes, it may make sense to split
> > these from the "trivial" conversions in a treewide patch. The chance of
> > needing a revert from the simple 1:1 conversions is much lower than the
> > need to revert by-hand changes.
> >
> > The Cocci script I suggested in my v1 review gets 80% of the first
> > patch, for example.
>
> I'll split things up into a mechanical step and a non-mechanical step.
> Good idea.
I'd also do something about the 'get_random_int() & 3' cases.
(ie remainder by 2^n-1)
These can be converted to 'get_random_u8() & 3' (etc).
So they only need one random byte (not 4) and no multiply.
Possibly something based on (the quickly typed, and not C):
#define get_random_below(val) [
if (builtin_constant(val))
BUILD_BUG_ON(!val || val > 0x1ull)
if (!(val & (val - 1)) {
if (val <= 0x100)
return get_random_u8() & (val - 1);
if (val <= 0x1)
return get_random_u16() & (val - 1);
return get_random_u32() & (val - 1);
}
}
BUILD_BUG_ON(sizeof (val) > 4);
return ((u64)get_random_u32() * val) >> 32;
}
get_random_below() is a much better name than prandom_u32_max().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 10:12:42AM -0700, Kees Cook wrote:
> On Fri, Oct 07, 2022 at 08:07:58AM -0600, Jason A. Donenfeld wrote:
> > On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
> > >
> > >
> > > Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > > > On 10/6/22, Christophe Leroy wrote:
> > > >>
> > > >>
> > > >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> > > >>>
> > > >>>
> > > >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> > > Hi Christophe,
> > >
> > > On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> > > wrote:
> > > > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > > >> The prandom_u32() function has been a deprecated inline wrapper
> > > >> around
> > > >> get_random_u32() for several releases now, and compiles down to the
> > > >> exact same code. Replace the deprecated wrapper with a direct call
> > > >> to
> > > >> the real function. The same also applies to get_random_int(),
> > > >> which is
> > > >> just a wrapper around get_random_u32().
> > > >>
> > > >> Reviewed-by: Kees Cook
> > > >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > > >> Acked-by: Chuck Lever # for nfsd
> > > >> Reviewed-by: Jan Kara # for ext4
> > > >> Signed-off-by: Jason A. Donenfeld
> > > >> ---
> > > >
> > > >> diff --git a/arch/powerpc/kernel/process.c
> > > >> b/arch/powerpc/kernel/process.c
> > > >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> > > >> --- a/arch/powerpc/kernel/process.c
> > > >> +++ b/arch/powerpc/kernel/process.c
> > > >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > > >> unsigned long arch_align_stack(unsigned long sp)
> > > >> {
> > > >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> > > >> randomize_va_space)
> > > >> - sp -= get_random_int() & ~PAGE_MASK;
> > > >> + sp -= get_random_u32() & ~PAGE_MASK;
> > > >> return sp & ~0xf;
> > > >
> > > > Isn't that a candidate for prandom_u32_max() ?
> > > >
> > > > Note that sp is deemed to be 16 bytes aligned at all time.
> > >
> > > Yes, probably. It seemed non-trivial to think about, so I didn't. But
> > > let's see here... maybe it's not too bad:
> > >
> > > If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> > > (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> > > thing? Is that accurate? And holds across platforms (this comes up a
> > > few places)? If so, I'll do that for a v4.
> > >
> > > >>>
> > > >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> > > >>>
> > > >>> /*
> > > >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if
> > > >>> we
> > > >>>* assign PAGE_MASK to a larger type it gets extended the way we
> > > >>> want
> > > >>>* (i.e. with 1s in the high bits)
> > > >>>*/
> > > >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> > > >>>
> > > >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> > > >>>
> > > >>>
> > > >>> So it would work I guess.
> > > >>
> > > >> But taking into account that sp must remain 16 bytes aligned, would it
> > > >> be better to do something like ?
> > > >>
> > > >>sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> > > >>
> > > >>return sp;
> > > >
> > > > Does this assume that sp is already aligned at the beginning of the
> > > > function? I'd assume from the function's name that this isn't the
> > > > case?
> > >
> > > Ah you are right, I overlooked it.
> >
> > So I think to stay on the safe side, I'm going to go with
> > `prandom_u32_max(PAGE_SIZE)`. Sound good?
>
> Given these kinds of less mechanical changes, it may make sense to split
> these from the "trivial" conversions in a treewide patch. The chance of
> needing a revert from the simple 1:1 conversions is much lower than the
> need to revert by-hand changes.
>
> The Cocci script I suggested in my v1 review gets 80% of the first
> patch, for example.
I'll split things up into a mechanical step and a non-mechanical step.
Good idea.
Jason
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 08:07:58AM -0600, Jason A. Donenfeld wrote:
> On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
> >
> >
> > Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > > On 10/6/22, Christophe Leroy wrote:
> > >>
> > >>
> > >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> > >>>
> > >>>
> > >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> > Hi Christophe,
> >
> > On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> > wrote:
> > > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > >> The prandom_u32() function has been a deprecated inline wrapper
> > >> around
> > >> get_random_u32() for several releases now, and compiles down to the
> > >> exact same code. Replace the deprecated wrapper with a direct call to
> > >> the real function. The same also applies to get_random_int(), which
> > >> is
> > >> just a wrapper around get_random_u32().
> > >>
> > >> Reviewed-by: Kees Cook
> > >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > >> Acked-by: Chuck Lever # for nfsd
> > >> Reviewed-by: Jan Kara # for ext4
> > >> Signed-off-by: Jason A. Donenfeld
> > >> ---
> > >
> > >> diff --git a/arch/powerpc/kernel/process.c
> > >> b/arch/powerpc/kernel/process.c
> > >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> > >> --- a/arch/powerpc/kernel/process.c
> > >> +++ b/arch/powerpc/kernel/process.c
> > >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > >> unsigned long arch_align_stack(unsigned long sp)
> > >> {
> > >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> > >> randomize_va_space)
> > >> - sp -= get_random_int() & ~PAGE_MASK;
> > >> + sp -= get_random_u32() & ~PAGE_MASK;
> > >> return sp & ~0xf;
> > >
> > > Isn't that a candidate for prandom_u32_max() ?
> > >
> > > Note that sp is deemed to be 16 bytes aligned at all time.
> >
> > Yes, probably. It seemed non-trivial to think about, so I didn't. But
> > let's see here... maybe it's not too bad:
> >
> > If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> > (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> > thing? Is that accurate? And holds across platforms (this comes up a
> > few places)? If so, I'll do that for a v4.
> >
> > >>>
> > >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> > >>>
> > >>> /*
> > >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
> > >>>* assign PAGE_MASK to a larger type it gets extended the way we want
> > >>>* (i.e. with 1s in the high bits)
> > >>>*/
> > >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> > >>>
> > >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> > >>>
> > >>>
> > >>> So it would work I guess.
> > >>
> > >> But taking into account that sp must remain 16 bytes aligned, would it
> > >> be better to do something like ?
> > >>
> > >> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> > >>
> > >> return sp;
> > >
> > > Does this assume that sp is already aligned at the beginning of the
> > > function? I'd assume from the function's name that this isn't the
> > > case?
> >
> > Ah you are right, I overlooked it.
>
> So I think to stay on the safe side, I'm going to go with
> `prandom_u32_max(PAGE_SIZE)`. Sound good?
Given these kinds of less mechanical changes, it may make sense to split
these from the "trivial" conversions in a treewide patch. The chance of
needing a revert from the simple 1:1 conversions is much lower than the
need to revert by-hand changes.
The Cocci script I suggested in my v1 review gets 80% of the first
patch, for example.
--
Kees Cook
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
>
>
> Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > On 10/6/22, Christophe Leroy wrote:
> >>
> >>
> >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> >>>
> >>>
> >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> Hi Christophe,
>
> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> wrote:
> > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> >> The prandom_u32() function has been a deprecated inline wrapper around
> >> get_random_u32() for several releases now, and compiles down to the
> >> exact same code. Replace the deprecated wrapper with a direct call to
> >> the real function. The same also applies to get_random_int(), which is
> >> just a wrapper around get_random_u32().
> >>
> >> Reviewed-by: Kees Cook
> >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> >> Acked-by: Chuck Lever # for nfsd
> >> Reviewed-by: Jan Kara # for ext4
> >> Signed-off-by: Jason A. Donenfeld
> >> ---
> >
> >> diff --git a/arch/powerpc/kernel/process.c
> >> b/arch/powerpc/kernel/process.c
> >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> >> --- a/arch/powerpc/kernel/process.c
> >> +++ b/arch/powerpc/kernel/process.c
> >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> >> unsigned long arch_align_stack(unsigned long sp)
> >> {
> >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> >> randomize_va_space)
> >> - sp -= get_random_int() & ~PAGE_MASK;
> >> + sp -= get_random_u32() & ~PAGE_MASK;
> >> return sp & ~0xf;
> >
> > Isn't that a candidate for prandom_u32_max() ?
> >
> > Note that sp is deemed to be 16 bytes aligned at all time.
>
> Yes, probably. It seemed non-trivial to think about, so I didn't. But
> let's see here... maybe it's not too bad:
>
> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> thing? Is that accurate? And holds across platforms (this comes up a
> few places)? If so, I'll do that for a v4.
>
> >>>
> >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> >>>
> >>> /*
> >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
> >>>* assign PAGE_MASK to a larger type it gets extended the way we want
> >>>* (i.e. with 1s in the high bits)
> >>>*/
> >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> >>>
> >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> >>>
> >>>
> >>> So it would work I guess.
> >>
> >> But taking into account that sp must remain 16 bytes aligned, would it
> >> be better to do something like ?
> >>
> >>sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> >>
> >>return sp;
> >
> > Does this assume that sp is already aligned at the beginning of the
> > function? I'd assume from the function's name that this isn't the
> > case?
>
> Ah you are right, I overlooked it.
So I think to stay on the safe side, I'm going to go with
`prandom_u32_max(PAGE_SIZE)`. Sound good?
Jason
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 10:53:44AM -0600, Jason A. Donenfeld wrote: > drivers/thunderbolt/xdomain.c | 2 +- Acked-by: Mika Westerberg
RE: [PATCH v3 3/5] treewide: use get_random_u32() when possible
From: Christophe Leroy > Sent: 06 October 2022 18:43 ... > But taking into account that sp must remain 16 bytes aligned, would it > be better to do something like ? > > sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4; That makes me think... If prandom_u32_max() is passed a (constant) power of 2 it doesn't need to do the multiply, it can just do a shift right. Doesn't it also always get a 32bit random value? So actually get_random_u32() & PAGE_MASK & ~0xf is faster! When PAGE_SIZE is 4k, PAGE_SIZE >> 4 is 256 so it could use: get_ramdom_u8() << 4 You also seem to have removed prandom_u32() in favour of get_random_u32() but have added more prandom_() functions. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> On 10/6/22, Christophe Leroy wrote:
>>
>>
>> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
>>>
>>>
>>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
Hi Christophe,
On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
wrote:
> Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
>> The prandom_u32() function has been a deprecated inline wrapper around
>> get_random_u32() for several releases now, and compiles down to the
>> exact same code. Replace the deprecated wrapper with a direct call to
>> the real function. The same also applies to get_random_int(), which is
>> just a wrapper around get_random_u32().
>>
>> Reviewed-by: Kees Cook
>> Acked-by: Toke Høiland-Jørgensen # for sch_cake
>> Acked-by: Chuck Lever # for nfsd
>> Reviewed-by: Jan Kara # for ext4
>> Signed-off-by: Jason A. Donenfeld
>> ---
>
>> diff --git a/arch/powerpc/kernel/process.c
>> b/arch/powerpc/kernel/process.c
>> index 0fbda89cd1bb..9c4c15afbbe8 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
>> unsigned long arch_align_stack(unsigned long sp)
>> {
>> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
>> randomize_va_space)
>> - sp -= get_random_int() & ~PAGE_MASK;
>> + sp -= get_random_u32() & ~PAGE_MASK;
>> return sp & ~0xf;
>
> Isn't that a candidate for prandom_u32_max() ?
>
> Note that sp is deemed to be 16 bytes aligned at all time.
Yes, probably. It seemed non-trivial to think about, so I didn't. But
let's see here... maybe it's not too bad:
If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
(PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
thing? Is that accurate? And holds across platforms (this comes up a
few places)? If so, I'll do that for a v4.
>>>
>>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
>>>
>>> /*
>>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
>>>* assign PAGE_MASK to a larger type it gets extended the way we want
>>>* (i.e. with 1s in the high bits)
>>>*/
>>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
>>>
>>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
>>>
>>>
>>> So it would work I guess.
>>
>> But taking into account that sp must remain 16 bytes aligned, would it
>> be better to do something like ?
>>
>> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
>>
>> return sp;
>
> Does this assume that sp is already aligned at the beginning of the
> function? I'd assume from the function's name that this isn't the
> case?
Ah you are right, I overlooked it.
Looking in more details, I see that all architectures that implement it
implement it almost the same way.
By the way, the comment in arch/um/kernel/process.c is overdated.
Most architectures AND the random value with ~PAGE_MASK, x86 and um use
%8192. Seems like at the time 2.6.12 was introduced into git, only i386
x86_64 and um had that function.
Maybe it is time for a cleanup and a refactoring ? Architectures would
just have to provide STACK_ALIGN just like loongarch does today, and we
could get a generic arch_align_stack() ?
Christophe
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On 10/6/22, Christophe Leroy wrote:
>
>
> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
>>
>>
>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
>>> Hi Christophe,
>>>
>>> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
>>> wrote:
Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function. The same also applies to get_random_int(), which is
> just a wrapper around get_random_u32().
>
> Reviewed-by: Kees Cook
> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> Acked-by: Chuck Lever # for nfsd
> Reviewed-by: Jan Kara # for ext4
> Signed-off-by: Jason A. Donenfeld
> ---
> diff --git a/arch/powerpc/kernel/process.c
> b/arch/powerpc/kernel/process.c
> index 0fbda89cd1bb..9c4c15afbbe8 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
>unsigned long arch_align_stack(unsigned long sp)
>{
>if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> randomize_va_space)
> - sp -= get_random_int() & ~PAGE_MASK;
> + sp -= get_random_u32() & ~PAGE_MASK;
>return sp & ~0xf;
Isn't that a candidate for prandom_u32_max() ?
Note that sp is deemed to be 16 bytes aligned at all time.
>>>
>>> Yes, probably. It seemed non-trivial to think about, so I didn't. But
>>> let's see here... maybe it's not too bad:
>>>
>>> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
>>> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
>>> thing? Is that accurate? And holds across platforms (this comes up a
>>> few places)? If so, I'll do that for a v4.
>>>
>>
>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
>>
>> /*
>> * Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
>> * assign PAGE_MASK to a larger type it gets extended the way we want
>> * (i.e. with 1s in the high bits)
>> */
>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
>>
>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
>>
>>
>> So it would work I guess.
>
> But taking into account that sp must remain 16 bytes aligned, would it
> be better to do something like ?
>
> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
>
> return sp;
Does this assume that sp is already aligned at the beginning of the
function? I'd assume from the function's name that this isn't the
case?
Jason
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
Hi Christophe,
On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
wrote:
Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
The prandom_u32() function has been a deprecated inline wrapper around
get_random_u32() for several releases now, and compiles down to the
exact same code. Replace the deprecated wrapper with a direct call to
the real function. The same also applies to get_random_int(), which is
just a wrapper around get_random_u32().
Reviewed-by: Kees Cook
Acked-by: Toke Høiland-Jørgensen # for sch_cake
Acked-by: Chuck Lever # for nfsd
Reviewed-by: Jan Kara # for ext4
Signed-off-by: Jason A. Donenfeld
---
diff --git a/arch/powerpc/kernel/process.c
b/arch/powerpc/kernel/process.c
index 0fbda89cd1bb..9c4c15afbbe8 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
unsigned long arch_align_stack(unsigned long sp)
{
if (!(current->personality & ADDR_NO_RANDOMIZE) &&
randomize_va_space)
- sp -= get_random_int() & ~PAGE_MASK;
+ sp -= get_random_u32() & ~PAGE_MASK;
return sp & ~0xf;
Isn't that a candidate for prandom_u32_max() ?
Note that sp is deemed to be 16 bytes aligned at all time.
Yes, probably. It seemed non-trivial to think about, so I didn't. But
let's see here... maybe it's not too bad:
If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
(PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
thing? Is that accurate? And holds across platforms (this comes up a
few places)? If so, I'll do that for a v4.
On powerpc it is always (from arch/powerpc/include/asm/page.h) :
/*
* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
* assign PAGE_MASK to a larger type it gets extended the way we want
* (i.e. with 1s in the high bits)
*/
#define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
#define PAGE_SIZE (1UL << PAGE_SHIFT)
So it would work I guess.
But taking into account that sp must remain 16 bytes aligned, would it
be better to do something like ?
sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
return sp;
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> Hi Christophe,
>
> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> wrote:
>> Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
>>> The prandom_u32() function has been a deprecated inline wrapper around
>>> get_random_u32() for several releases now, and compiles down to the
>>> exact same code. Replace the deprecated wrapper with a direct call to
>>> the real function. The same also applies to get_random_int(), which is
>>> just a wrapper around get_random_u32().
>>>
>>> Reviewed-by: Kees Cook
>>> Acked-by: Toke Høiland-Jørgensen # for sch_cake
>>> Acked-by: Chuck Lever # for nfsd
>>> Reviewed-by: Jan Kara # for ext4
>>> Signed-off-by: Jason A. Donenfeld
>>> ---
>>
>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>> index 0fbda89cd1bb..9c4c15afbbe8 100644
>>> --- a/arch/powerpc/kernel/process.c
>>> +++ b/arch/powerpc/kernel/process.c
>>> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
>>>unsigned long arch_align_stack(unsigned long sp)
>>>{
>>>if (!(current->personality & ADDR_NO_RANDOMIZE) &&
>>> randomize_va_space)
>>> - sp -= get_random_int() & ~PAGE_MASK;
>>> + sp -= get_random_u32() & ~PAGE_MASK;
>>>return sp & ~0xf;
>>
>> Isn't that a candidate for prandom_u32_max() ?
>>
>> Note that sp is deemed to be 16 bytes aligned at all time.
>
> Yes, probably. It seemed non-trivial to think about, so I didn't. But
> let's see here... maybe it's not too bad:
>
> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> thing? Is that accurate? And holds across platforms (this comes up a
> few places)? If so, I'll do that for a v4.
>
On powerpc it is always (from arch/powerpc/include/asm/page.h) :
/*
* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
* assign PAGE_MASK to a larger type it gets extended the way we want
* (i.e. with 1s in the high bits)
*/
#define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
#define PAGE_SIZE (1UL << PAGE_SHIFT)
So it would work I guess.
Christophe
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Hi Christophe,
On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
wrote:
> Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > The prandom_u32() function has been a deprecated inline wrapper around
> > get_random_u32() for several releases now, and compiles down to the
> > exact same code. Replace the deprecated wrapper with a direct call to
> > the real function. The same also applies to get_random_int(), which is
> > just a wrapper around get_random_u32().
> >
> > Reviewed-by: Kees Cook
> > Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > Acked-by: Chuck Lever # for nfsd
> > Reviewed-by: Jan Kara # for ext4
> > Signed-off-by: Jason A. Donenfeld
> > ---
>
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index 0fbda89cd1bb..9c4c15afbbe8 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > unsigned long arch_align_stack(unsigned long sp)
> > {
> > if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> > - sp -= get_random_int() & ~PAGE_MASK;
> > + sp -= get_random_u32() & ~PAGE_MASK;
> > return sp & ~0xf;
>
> Isn't that a candidate for prandom_u32_max() ?
>
> Note that sp is deemed to be 16 bytes aligned at all time.
Yes, probably. It seemed non-trivial to think about, so I didn't. But
let's see here... maybe it's not too bad:
If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
(PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
thing? Is that accurate? And holds across platforms (this comes up a
few places)? If so, I'll do that for a v4.
Jason
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function. The same also applies to get_random_int(), which is
> just a wrapper around get_random_u32().
>
> Reviewed-by: Kees Cook
> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> Acked-by: Chuck Lever # for nfsd
> Reviewed-by: Jan Kara # for ext4
> Signed-off-by: Jason A. Donenfeld
> ---
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 0fbda89cd1bb..9c4c15afbbe8 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> unsigned long arch_align_stack(unsigned long sp)
> {
> if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> - sp -= get_random_int() & ~PAGE_MASK;
> + sp -= get_random_u32() & ~PAGE_MASK;
> return sp & ~0xf;
Isn't that a candidate for prandom_u32_max() ?
Note that sp is deemed to be 16 bytes aligned at all time.
Christophe
