[PATCH] ARM: Remove address checking for MMUless devices

[PATCH] ARM: Remove address checking for MMUless devices

[Yanjun Yang]

Commit 169f9102f9198b ("ARM: 9350/1: fault:
Implement copy_from_kernel_nofault_allowed()") added the function to check
address before use. However, for devices without MMU, addr > TASK_SIZE
will always fail.  This patch move this function after the #ifdef
CONFIG_MMU statement.

Also reported at https://bugzilla.kernel.org/show_bug.cgi?id=218953

Signed-off-by: Yanjun Yang <yangyj.ee@gmail.com>
---
 arch/arm/mm/fault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 67c425341a95..ab01b51de559 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -25,6 +25,8 @@
 
 #include "fault.h"
 
+#ifdef CONFIG_MMU
+
 bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
 {
 	unsigned long addr = (unsigned long)unsafe_src;
@@ -32,8 +34,6 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
 	return addr >= TASK_SIZE && ULONG_MAX - addr >= size;
 }
 
-#ifdef CONFIG_MMU
-
 /*
  * This is useful to dump out the page tables associated with
  * 'addr' in mm 'mm'.
-- 
2.45.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Yanjun Yang]

Hi,
Apologies for not CCing the relevant maintainers in my previous email.

On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
>
> Commit 169f9102f9198b ("ARM: 9350/1: fault:
> Implement copy_from_kernel_nofault_allowed()") added the function to check
> address before use. However, for devices without MMU, addr > TASK_SIZE
> will always fail.  This patch move this function after the #ifdef
> CONFIG_MMU statement.
>
> Also reported at https://bugzilla.kernel.org/show_bug.cgi?id=218953
>
> Signed-off-by: Yanjun Yang <yangyj.ee@gmail.com>
> ---
>  arch/arm/mm/fault.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index 67c425341a95..ab01b51de559 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -25,6 +25,8 @@
>
>  #include "fault.h"
>
> +#ifdef CONFIG_MMU
> +
>  bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
>  {
>         unsigned long addr = (unsigned long)unsafe_src;
> @@ -32,8 +34,6 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
>         return addr >= TASK_SIZE && ULONG_MAX - addr >= size;
>  }
>
> -#ifdef CONFIG_MMU
> -
>  /*
>   * This is useful to dump out the page tables associated with
>   * 'addr' in mm 'mm'.
> --
> 2.45.2
>

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Ard Biesheuvel]

On Wed, 12 Jun 2024 at 03:26, Yanjun Yang <yangyj.ee@gmail.com> wrote:
>
> Hi,
> Apologies for not CCing the relevant maintainers in my previous email.
>
> On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
> >
> > Commit 169f9102f9198b ("ARM: 9350/1: fault:
> > Implement copy_from_kernel_nofault_allowed()") added the function to check
> > address before use. However, for devices without MMU, addr > TASK_SIZE
> > will always fail.

Is that true? Doesn't it depend on the physical memory layout of the
platform in question?

>  This patch move this function after the #ifdef
> > CONFIG_MMU statement.
> >
> > Also reported at https://bugzilla.kernel.org/show_bug.cgi?id=218953
> >
> > Signed-off-by: Yanjun Yang <yangyj.ee@gmail.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> > ---
> >  arch/arm/mm/fault.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > index 67c425341a95..ab01b51de559 100644
> > --- a/arch/arm/mm/fault.c
> > +++ b/arch/arm/mm/fault.c
> > @@ -25,6 +25,8 @@
> >
> >  #include "fault.h"
> >
> > +#ifdef CONFIG_MMU
> > +
> >  bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
> >  {
> >         unsigned long addr = (unsigned long)unsafe_src;
> > @@ -32,8 +34,6 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
> >         return addr >= TASK_SIZE && ULONG_MAX - addr >= size;
> >  }
> >
> > -#ifdef CONFIG_MMU
> > -
> >  /*
> >   * This is useful to dump out the page tables associated with
> >   * 'addr' in mm 'mm'.
> > --
> > 2.45.2
> >

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Yanjun Yang]

On Wed, Jun 12, 2024 at 2:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 12 Jun 2024 at 03:26, Yanjun Yang <yangyj.ee@gmail.com> wrote:
> >
> > Hi,
> > Apologies for not CCing the relevant maintainers in my previous email.
> >
> > On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
> > >
> > > Commit 169f9102f9198b ("ARM: 9350/1: fault:
> > > Implement copy_from_kernel_nofault_allowed()") added the function to check
> > > address before use. However, for devices without MMU, addr > TASK_SIZE
> > > will always fail.
>
> Is that true? Doesn't it depend on the physical memory layout of the
> platform in question?
>

I only checked the ARM architecture, in arch/arm/include/asm/memory.h
TASK_SIZE is
defined as 0xffffffff when CONFIG_MMU is not defined.  Following is
the code snippet.
/*
 * The limitation of user task size can grow up to the end of free ram region.
 * It is difficult to define and perhaps will never meet the original meaning
 * of this define that was meant to.
 * Fortunately, there is no reference for this in noMMU mode, for now.
 */
#define TASK_SIZE               UL(0xffffffff)


> >  This patch move this function after the #ifdef
> > > CONFIG_MMU statement.
> > >
> > > Also reported at https://bugzilla.kernel.org/show_bug.cgi?id=218953
> > >
> > > Signed-off-by: Yanjun Yang <yangyj.ee@gmail.com>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> > > ---
> > >  arch/arm/mm/fault.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > > index 67c425341a95..ab01b51de559 100644
> > > --- a/arch/arm/mm/fault.c
> > > +++ b/arch/arm/mm/fault.c
> > > @@ -25,6 +25,8 @@
> > >
> > >  #include "fault.h"
> > >
> > > +#ifdef CONFIG_MMU
> > > +
> > >  bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
> > >  {
> > >         unsigned long addr = (unsigned long)unsafe_src;
> > > @@ -32,8 +34,6 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
> > >         return addr >= TASK_SIZE && ULONG_MAX - addr >= size;
> > >  }
> > >
> > > -#ifdef CONFIG_MMU
> > > -
> > >  /*
> > >   * This is useful to dump out the page tables associated with
> > >   * 'addr' in mm 'mm'.
> > > --
> > > 2.45.2
> > >

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Ard Biesheuvel]

On Wed, 12 Jun 2024 at 09:01, Yanjun Yang <yangyj.ee@gmail.com> wrote:
>
> On Wed, Jun 12, 2024 at 2:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 12 Jun 2024 at 03:26, Yanjun Yang <yangyj.ee@gmail.com> wrote:
> > >
> > > Hi,
> > > Apologies for not CCing the relevant maintainers in my previous email.
> > >
> > > On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
> > > >
> > > > Commit 169f9102f9198b ("ARM: 9350/1: fault:
> > > > Implement copy_from_kernel_nofault_allowed()") added the function to check
> > > > address before use. However, for devices without MMU, addr > TASK_SIZE
> > > > will always fail.
> >
> > Is that true? Doesn't it depend on the physical memory layout of the
> > platform in question?
> >
>
> I only checked the ARM architecture, in arch/arm/include/asm/memory.h
> TASK_SIZE is
> defined as 0xffffffff when CONFIG_MMU is not defined.  Following is
> the code snippet.
> /*
>  * The limitation of user task size can grow up to the end of free ram region.
>  * It is difficult to define and perhaps will never meet the original meaning
>  * of this define that was meant to.
>  * Fortunately, there is no reference for this in noMMU mode, for now.
>  */
> #define TASK_SIZE               UL(0xffffffff)
>

OK, I stand corrected. Thanks for the explanation.

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Linux regression tracking (Thorsten Leemhuis)]

On 12.06.24 03:25, Yanjun Yang wrote:
> Apologies for not CCing the relevant maintainers in my previous email.
> 
> On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
>>
>> Commit 169f9102f9198b ("ARM: 9350/1: fault:
>> Implement copy_from_kernel_nofault_allowed()") added the function to check
>> address before use. However, for devices without MMU, addr > TASK_SIZE
>> will always fail.  This patch move this function after the #ifdef
>> CONFIG_MMU statement.

What happened to this fix regression for a 6.9 regression? From here it
looks like it fell through the cracks, but I might be missing something.

>> Also reported at https://bugzilla.kernel.org/show_bug.cgi?id=218953

Side note: this afaics ideally should be:

 Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218953

Ciao, Thorsten

>> Signed-off-by: Yanjun Yang <yangyj.ee@gmail.com>
>> ---
>>  arch/arm/mm/fault.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
>> index 67c425341a95..ab01b51de559 100644
>> --- a/arch/arm/mm/fault.c
>> +++ b/arch/arm/mm/fault.c
>> @@ -25,6 +25,8 @@
>>
>>  #include "fault.h"
>>
>> +#ifdef CONFIG_MMU
>> +
>>  bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
>>  {
>>         unsigned long addr = (unsigned long)unsafe_src;
>> @@ -32,8 +34,6 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
>>         return addr >= TASK_SIZE && ULONG_MAX - addr >= size;
>>  }
>>
>> -#ifdef CONFIG_MMU
>> -
>>  /*
>>   * This is useful to dump out the page tables associated with
>>   * 'addr' in mm 'mm'.
>> --
>> 2.45.2
>>
> 

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Kees Cook]

On Mon, Jul 01, 2024 at 01:54:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 12.06.24 03:25, Yanjun Yang wrote:
> > Apologies for not CCing the relevant maintainers in my previous email.
> > 
> > On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
> >>
> >> Commit 169f9102f9198b ("ARM: 9350/1: fault:
> >> Implement copy_from_kernel_nofault_allowed()") added the function to check
> >> address before use. However, for devices without MMU, addr > TASK_SIZE
> >> will always fail.  This patch move this function after the #ifdef
> >> CONFIG_MMU statement.
> 
> What happened to this fix regression for a 6.9 regression? From here it
> looks like it fell through the cracks, but I might be missing something.

This patch can be sent through rmk's ARM patch tracker, but if it's
urgent, I could take it via my tree? It has Ard's Ack.

-- 
Kees Cook

Re: [PATCH] ARM: Remove address checking for MMUless devices

[Russell King (Oracle)]

On Mon, Jul 01, 2024 at 12:17:10PM -0700, Kees Cook wrote:
> On Mon, Jul 01, 2024 at 01:54:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> > On 12.06.24 03:25, Yanjun Yang wrote:
> > > Apologies for not CCing the relevant maintainers in my previous email.
> > > 
> > > On Tue, Jun 11, 2024 at 6:09 PM Yanjun Yang <yangyj.ee@gmail.com> wrote:
> > >>
> > >> Commit 169f9102f9198b ("ARM: 9350/1: fault:
> > >> Implement copy_from_kernel_nofault_allowed()") added the function to check
> > >> address before use. However, for devices without MMU, addr > TASK_SIZE
> > >> will always fail.  This patch move this function after the #ifdef
> > >> CONFIG_MMU statement.
> > 
> > What happened to this fix regression for a 6.9 regression? From here it
> > looks like it fell through the cracks, but I might be missing something.
> 
> This patch can be sent through rmk's ARM patch tracker, but if it's
> urgent, I could take it via my tree? It has Ard's Ack.

I've been so busy with a high priority work issue that I've not been
aware of this email thread until I saw it tonight (because I haven't
been able to read much email!)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!