[PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[Feng Liu]

From: Alexey Dobriyan <adobriyan@gmail.com>

[ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]

ELF loader uses "randomize_va_space" twice. It is sysctl and can change
at any moment, so 2 loads could see 2 different values in theory with
unpredictable consequences.

Issue exactly one load for consistent value across one exec.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
Signed-off-by: He Zhe <Zhe.He@windriver.com>
---
Verified the build test.
---
 fs/binfmt_elf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 11dc833ca2c4..a1f0dff2f818 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1008,7 +1008,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	if (elf_read_implies_exec(*elf_ex, executable_stack))
 		current->personality |= READ_IMPLIES_EXEC;
 
-	if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
+	const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
+	if (!(current->personality & ADDR_NO_RANDOMIZE) && snapshot_randomize_va_space)
 		current->flags |= PF_RANDOMIZE;
 
 	setup_new_exec(bprm);
@@ -1278,7 +1279,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	mm->end_data = end_data;
 	mm->start_stack = bprm->p;
 
-	if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
+	if ((current->flags & PF_RANDOMIZE) && (snapshot_randomize_va_space > 1)) {
 		/*
 		 * For architectures with ELF randomization, when executing
 		 * a loader directly (i.e. no interpreter listed in ELF
-- 
2.34.1

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

✅ All tests passed successfully. No issues detected.
No action required from the submitter.

The upstream commit SHA1 provided is correct: 2a97388a807b6ab5538aa8f8537b2463c6988bd2

WARNING: Author mismatch between patch and upstream commit:
Backport author: Feng Liu<Feng.Liu3@windriver.com>
Commit author: Alexey Dobriyan<adobriyan@gmail.com>

Status in newer kernel trees:
6.14.y | Present (exact SHA1)
6.12.y | Present (exact SHA1)
6.6.y | Present (different SHA1: 53f17409abf6)
6.1.y | Present (different SHA1: 1f81d51141a2)
5.15.y | Not found

Note: The patch differs from the upstream commit:
---
1:  2a97388a807b6 ! 1:  795c22f390cda ELF: fix kernel.randomize_va_space double read
    @@ Metadata
      ## Commit message ##
         ELF: fix kernel.randomize_va_space double read
     
    +    [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
    +
         ELF loader uses "randomize_va_space" twice. It is sysctl and can change
         at any moment, so 2 loads could see 2 different values in theory with
         unpredictable consequences.
    @@ Commit message
         Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
         Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
         Signed-off-by: Kees Cook <kees@kernel.org>
    +    Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
    +    Signed-off-by: He Zhe <Zhe.He@windriver.com>
     
      ## fs/binfmt_elf.c ##
     @@ fs/binfmt_elf.c: static int load_elf_binary(struct linux_binprm *bprm)
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.10.y       |  Success    |  Success   |
| stable/linux-5.15.y       |  Success    |  Success   |

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[Greg KH]

On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> 
> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
> 
> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
> at any moment, so 2 loads could see 2 different values in theory with
> unpredictable consequences.
> 
> Issue exactly one load for consistent value across one exec.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
> Signed-off-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
> Signed-off-by: He Zhe <Zhe.He@windriver.com>
> ---
> Verified the build test.

No you did not!  This breaks the build.

This is really really annoying as it breaks the workflow on our side
when you submit code that does not work at all.

Please go and retest all of the outstanding commits that you all have
submitted and fix them up and resend them.  I'm dropping all of the rest
of them from my pending queue as this shows a total lack of testing
happening which implies that I can't trust any of these at all.

And I want you all to prove that you have actually tested the code, not
just this bland "Verified the build test" which is a _very_ low bar,
that is not even happening here at all :(

greg k-h

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[He Zhe]

On 2025/5/20 19:25, Greg KH wrote:
> On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
>> From: Alexey Dobriyan <adobriyan@gmail.com>
>>
>> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
>>
>> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
>> at any moment, so 2 loads could see 2 different values in theory with
>> unpredictable consequences.
>>
>> Issue exactly one load for consistent value across one exec.
>>
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
>> Signed-off-by: Kees Cook <kees@kernel.org>
>> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
>> Signed-off-by: He Zhe <Zhe.He@windriver.com>
>> ---
>> Verified the build test.
> No you did not!  This breaks the build.
>
> This is really really annoying as it breaks the workflow on our side
> when you submit code that does not work at all.
>
> Please go and retest all of the outstanding commits that you all have
> submitted and fix them up and resend them.  I'm dropping all of the rest
> of them from my pending queue as this shows a total lack of testing
> happening which implies that I can't trust any of these at all.
>
> And I want you all to prove that you have actually tested the code, not
> just this bland "Verified the build test" which is a _very_ low bar,
> that is not even happening here at all :(

Sorry for any inconvenience.

We did do some build test on Ubuntu22.04 with the default GCC 11.4.0 and
defconfig on an x86_64 machine against the latest linux-stable before sending
the patch out. And we just redid the build test and caught below warning that
we missed before:

../fs/binfmt_elf.c: In function ‘load_elf_binary’:
../fs/binfmt_elf.c:1011:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1011 |         const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
      |   

Just to be clear, is this the issue that breaks the build from your side?

We just used the default config and didn't manually enable -WERROR which is
disabled by default for 5.10 and 5.15. After searching around we feel that
we should have enabled it as suggested by
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9080ba4a6ec56447f263082825a4fddb873316b
even for 5.10 and 5.15, so that such case wouldn't go unnoticed.

And as you mentioned in another thread, we will definitely enlarge the test
coverage and provide more details, for example:

Machine: x86_64
OS: Ubuntu24.04, Ubuntu22.04, ...
GCC: 11.04, ...
Tree: https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/,
Branch: linux-6.12.y, ...
Commands: make allyesconfig, make bzImage, ...

for the first step and then introduce some automation and provide public link
containing more details.


Thanks,
Zhe

>
> greg k-h

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[He Zhe]

On 2025/5/22 15:40, He Zhe wrote:
>
> On 2025/5/20 19:25, Greg KH wrote:
>> On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
>>> From: Alexey Dobriyan <adobriyan@gmail.com>
>>>
>>> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
>>>
>>> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
>>> at any moment, so 2 loads could see 2 different values in theory with
>>> unpredictable consequences.
>>>
>>> Issue exactly one load for consistent value across one exec.
>>>
>>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>>> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
>>> Signed-off-by: Kees Cook <kees@kernel.org>
>>> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
>>> Signed-off-by: He Zhe <Zhe.He@windriver.com>
>>> ---
>>> Verified the build test.
>> No you did not!  This breaks the build.
>>
>> This is really really annoying as it breaks the workflow on our side
>> when you submit code that does not work at all.
>>
>> Please go and retest all of the outstanding commits that you all have
>> submitted and fix them up and resend them.  I'm dropping all of the rest
>> of them from my pending queue as this shows a total lack of testing
>> happening which implies that I can't trust any of these at all.
>>
>> And I want you all to prove that you have actually tested the code, not
>> just this bland "Verified the build test" which is a _very_ low bar,
>> that is not even happening here at all :(
> Sorry for any inconvenience.
>
> We did do some build test on Ubuntu22.04 with the default GCC 11.4.0 and
> defconfig on an x86_64 machine against the latest linux-stable before sending
> the patch out. And we just redid the build test and caught below warning that
> we missed before:
>
> ../fs/binfmt_elf.c: In function ‘load_elf_binary’:
> ../fs/binfmt_elf.c:1011:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>  1011 |         const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
>       |   
>
> Just to be clear, is this the issue that breaks the build from your side?
>
> We just used the default config and didn't manually enable -WERROR which is
> disabled by default for 5.10 and 5.15. After searching around we feel that
> we should have enabled it as suggested by
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9080ba4a6ec56447f263082825a4fddb873316b
> even for 5.10 and 5.15, so that such case wouldn't go unnoticed.

BTW, such case is also missed by our backport helper bot
https://lore.kernel.org/all/20250511221746-17a0e7ea300c9d83@stable.kernel.org/

Regards,
Zhe

>
> And as you mentioned in another thread, we will definitely enlarge the test
> coverage and provide more details, for example:
>
> Machine: x86_64
> OS: Ubuntu24.04, Ubuntu22.04, ...
> GCC: 11.04, ...
> Tree: https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/,
> Branch: linux-6.12.y, ...
> Commands: make allyesconfig, make bzImage, ...
>
> for the first step and then introduce some automation and provide public link
> containing more details.
>
>
> Thanks,
> Zhe
>
>> greg k-h

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[Greg KH]

On Thu, May 22, 2025 at 03:40:16PM +0800, He Zhe wrote:
> 
> 
> On 2025/5/20 19:25, Greg KH wrote:
> > On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
> >> From: Alexey Dobriyan <adobriyan@gmail.com>
> >>
> >> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
> >>
> >> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
> >> at any moment, so 2 loads could see 2 different values in theory with
> >> unpredictable consequences.
> >>
> >> Issue exactly one load for consistent value across one exec.
> >>
> >> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> >> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
> >> Signed-off-by: Kees Cook <kees@kernel.org>
> >> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
> >> Signed-off-by: He Zhe <Zhe.He@windriver.com>
> >> ---
> >> Verified the build test.
> > No you did not!  This breaks the build.
> >
> > This is really really annoying as it breaks the workflow on our side
> > when you submit code that does not work at all.
> >
> > Please go and retest all of the outstanding commits that you all have
> > submitted and fix them up and resend them.  I'm dropping all of the rest
> > of them from my pending queue as this shows a total lack of testing
> > happening which implies that I can't trust any of these at all.
> >
> > And I want you all to prove that you have actually tested the code, not
> > just this bland "Verified the build test" which is a _very_ low bar,
> > that is not even happening here at all :(
> 
> Sorry for any inconvenience.
> 
> We did do some build test on Ubuntu22.04 with the default GCC 11.4.0 and
> defconfig on an x86_64 machine against the latest linux-stable before sending
> the patch out. And we just redid the build test and caught below warning that
> we missed before:

That is a very old version of gcc, and why are you using ubuntu when
this all should be tested on your version of Linux as that's what you
are backporting these patches for, right?  Shouldn't you be doing this
work for the portions of the kernel that you are actually using so that
you can properly test this stuff?

> ../fs/binfmt_elf.c: In function ‘load_elf_binary’:
> ../fs/binfmt_elf.c:1011:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>  1011 |         const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
>       |   

Do you think adding a new warning is ok?

> Just to be clear, is this the issue that breaks the build from your side?

I don't remember, given that it was many hundreds of patches ago.  But
probably.  Try it yourself and see!

> We just used the default config and didn't manually enable -WERROR which is
> disabled by default for 5.10 and 5.15. After searching around we feel that
> we should have enabled it as suggested by
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9080ba4a6ec56447f263082825a4fddb873316b
> even for 5.10 and 5.15, so that such case wouldn't go unnoticed.

Default configs for x86 are very limited, please do better testing.

greg k-h

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[He Zhe]

On 2025/5/22 16:36, Greg KH wrote:
> On Thu, May 22, 2025 at 03:40:16PM +0800, He Zhe wrote:
>>
>> On 2025/5/20 19:25, Greg KH wrote:
>>> On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
>>>> From: Alexey Dobriyan <adobriyan@gmail.com>
>>>>
>>>> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
>>>>
>>>> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
>>>> at any moment, so 2 loads could see 2 different values in theory with
>>>> unpredictable consequences.
>>>>
>>>> Issue exactly one load for consistent value across one exec.
>>>>
>>>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>>>> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
>>>> Signed-off-by: Kees Cook <kees@kernel.org>
>>>> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
>>>> Signed-off-by: He Zhe <Zhe.He@windriver.com>
>>>> ---
>>>> Verified the build test.
>>> No you did not!  This breaks the build.
>>>
>>> This is really really annoying as it breaks the workflow on our side
>>> when you submit code that does not work at all.
>>>
>>> Please go and retest all of the outstanding commits that you all have
>>> submitted and fix them up and resend them.  I'm dropping all of the rest
>>> of them from my pending queue as this shows a total lack of testing
>>> happening which implies that I can't trust any of these at all.
>>>
>>> And I want you all to prove that you have actually tested the code, not
>>> just this bland "Verified the build test" which is a _very_ low bar,
>>> that is not even happening here at all :(
>> Sorry for any inconvenience.
>>
>> We did do some build test on Ubuntu22.04 with the default GCC 11.4.0 and
>> defconfig on an x86_64 machine against the latest linux-stable before sending
>> the patch out. And we just redid the build test and caught below warning that
>> we missed before:
> That is a very old version of gcc, and why are you using ubuntu when
> this all should be tested on your version of Linux as that's what you
> are backporting these patches for, right?  Shouldn't you be doing this
> work for the portions of the kernel that you are actually using so that
> you can properly test this stuff?

Yes, we tested on our own version too, but also have to test build with the tree we're submitting
the patch to. So we use ubuntu22.04 for the building machine, not the one we want to replace the
kernel with.

>
>> ../fs/binfmt_elf.c: In function ‘load_elf_binary’:
>> ../fs/binfmt_elf.c:1011:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>  1011 |         const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
>>       |   
> Do you think adding a new warning is ok?

Of course not, we just missed this one.

>
>> Just to be clear, is this the issue that breaks the build from your side?
> I don't remember, given that it was many hundreds of patches ago.  But
> probably.  Try it yourself and see!
>
>> We just used the default config and didn't manually enable -WERROR which is
>> disabled by default for 5.10 and 5.15. After searching around we feel that
>> we should have enabled it as suggested by
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9080ba4a6ec56447f263082825a4fddb873316b
>> even for 5.10 and 5.15, so that such case wouldn't go unnoticed.
> Default configs for x86 are very limited, please do better testing.

OK, will do.

Regards,
Zhe

>
> greg k-h

Re: [PATCH 5.10.y/5.15.y] ELF: fix kernel.randomize_va_space double read

[Greg KH]

On Thu, May 22, 2025 at 05:26:33PM +0800, He Zhe wrote:
> 
> 
> On 2025/5/22 16:36, Greg KH wrote:
> > On Thu, May 22, 2025 at 03:40:16PM +0800, He Zhe wrote:
> >>
> >> On 2025/5/20 19:25, Greg KH wrote:
> >>> On Fri, May 09, 2025 at 02:14:15PM +0800, Feng Liu wrote:
> >>>> From: Alexey Dobriyan <adobriyan@gmail.com>
> >>>>
> >>>> [ Upstream commit 2a97388a807b6ab5538aa8f8537b2463c6988bd2 ]
> >>>>
> >>>> ELF loader uses "randomize_va_space" twice. It is sysctl and can change
> >>>> at any moment, so 2 loads could see 2 different values in theory with
> >>>> unpredictable consequences.
> >>>>
> >>>> Issue exactly one load for consistent value across one exec.
> >>>>
> >>>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> >>>> Link: https://lore.kernel.org/r/3329905c-7eb8-400a-8f0a-d87cff979b5b@p183
> >>>> Signed-off-by: Kees Cook <kees@kernel.org>
> >>>> Signed-off-by: Feng Liu <Feng.Liu3@windriver.com>
> >>>> Signed-off-by: He Zhe <Zhe.He@windriver.com>
> >>>> ---
> >>>> Verified the build test.
> >>> No you did not!  This breaks the build.
> >>>
> >>> This is really really annoying as it breaks the workflow on our side
> >>> when you submit code that does not work at all.
> >>>
> >>> Please go and retest all of the outstanding commits that you all have
> >>> submitted and fix them up and resend them.  I'm dropping all of the rest
> >>> of them from my pending queue as this shows a total lack of testing
> >>> happening which implies that I can't trust any of these at all.
> >>>
> >>> And I want you all to prove that you have actually tested the code, not
> >>> just this bland "Verified the build test" which is a _very_ low bar,
> >>> that is not even happening here at all :(
> >> Sorry for any inconvenience.
> >>
> >> We did do some build test on Ubuntu22.04 with the default GCC 11.4.0 and
> >> defconfig on an x86_64 machine against the latest linux-stable before sending
> >> the patch out. And we just redid the build test and caught below warning that
> >> we missed before:
> > That is a very old version of gcc, and why are you using ubuntu when
> > this all should be tested on your version of Linux as that's what you
> > are backporting these patches for, right?  Shouldn't you be doing this
> > work for the portions of the kernel that you are actually using so that
> > you can properly test this stuff?
> 
> Yes, we tested on our own version too, but also have to test build with the tree we're submitting
> the patch to. So we use ubuntu22.04 for the building machine, not the one we want to replace the
> kernel with.

But this wasn't built, or tested, on your target system.  Otherwise your
own internal CI would have caught this before even considering it for
submission to us.

> >> ../fs/binfmt_elf.c: In function ‘load_elf_binary’:
> >> ../fs/binfmt_elf.c:1011:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> >>  1011 |         const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
> >>       |   
> > Do you think adding a new warning is ok?
> 
> Of course not, we just missed this one.

It was not "just this one" that caused me to get frustrated and delete
all of your patches from the review queue.

Go off and rework your process for all of this please, otherwise I will
have to continue to just ignore these patch submissions from this
email domain.

greg k-h