| Message ID | 20240611100947.32241-1-yangyj.ee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Remove address checking for MMUless devices | expand |
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 >
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 > >
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 > > >
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.
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 >> >
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.
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!)
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'.
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(-)