Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Mon, Feb 21, 2022 at 5:25 AM Thomas Bogendoerfer wrote: > > With this patch [ .. snip snip ..] > I at least get my simple test cases fixed, but I'm not sure this is > correct. I think you really want to do that anyway, just to get things like wild kernel pointers right (ie think get_kernel_nofault() and friends for ftrace etc). They shouldn't happen in any normal situation, but those kinds of unverified pointers is why we _have_ get_kernel_nofault() in the first place. On x86-64, the roughly equivalent situation is that addresses that aren't in canonical format do not take a #PF (page fault), they take a #GP (general protection) fault. So I think you want to do that fixup_exception() for any possible addresses. > Is there a reason to not also #define TASK_SIZE_MAX __UA_LIMIT like > for the 32bit case ? I would suggest against using a non-constant TASK_SIZE_MAX. Being constant is literally one reason why it exists, when TASK_SIZE itself has often been about other things (ie "32-bit process"). Having to load variables for things like get_user() is annoying, if you could do it with a simple constant instead (where that "simple" part is to avoid having to load big values from a constant pool - often constants like "high bit set" can be loaded and compared against more efficiently). Linus ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Wed, Feb 23, 2022 at 8:41 AM Thomas Bogendoerfer wrote: > > On Wed, Feb 16, 2022 at 02:13:23PM +0100, Arnd Bergmann wrote: > > diff --git a/arch/mips/include/asm/uaccess.h > > b/arch/mips/include/asm/uaccess.h > > index db9a8e002b62..d7c89dc3426c 100644 > > --- a/arch/mips/include/asm/uaccess.h > > +++ b/arch/mips/include/asm/uaccess.h > > @@ -19,6 +19,7 @@ > > #ifdef CONFIG_32BIT > > > > #define __UA_LIMIT 0x8000UL > > +#define TASK_SIZE_MAX__UA_LIMIT > > using KSEG0 instead would IMHO be the better choice. This gives the > chance to remove __UA_LIMIT completly after cleaning up ptrace.c Ok, changed now. Arnd ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Wed, Feb 16, 2022 at 02:13:23PM +0100, Arnd Bergmann wrote: > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > index db9a8e002b62..d7c89dc3426c 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -19,6 +19,7 @@ > #ifdef CONFIG_32BIT > > #define __UA_LIMIT 0x8000UL > +#define TASK_SIZE_MAX__UA_LIMIT using KSEG0 instead would IMHO be the better choice. This gives the chance to remove __UA_LIMIT completly after cleaning up ptrace.c Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea.[ RFC1925, 2.3 ] ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Mon, Feb 21, 2022 at 03:31:23PM +0100, Arnd Bergmann wrote: > I'll update it this way, otherwise I'd need help in form of a patch > that changes the exception handling so __get_user/__put_user > also return -EFAULT for an address error. https://lore.kernel.org/all/20220222155345.138861-1-tsbog...@alpha.franken.de/ That does the trick. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea.[ RFC1925, 2.3 ] ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Mon, Feb 21, 2022 at 03:31:23PM +0100, Arnd Bergmann wrote: > On Mon, Feb 21, 2022 at 2:24 PM Thomas Bogendoerfer > wrote: > > On Wed, Feb 16, 2022 at 02:13:23PM +0100, Arnd Bergmann wrote: > > > > > > diff --git a/arch/mips/include/asm/uaccess.h > > > b/arch/mips/include/asm/uaccess.h > > > index db9a8e002b62..d7c89dc3426c 100644 > > > > this doesn't work. For every access above maximum implemented virtual > > address > > space of the CPU an address error will be issued, but not a TLB miss. > > And address error isn't able to handle this situation. > > Ah, so the __ex_table entry only catches TLB misses? no, but there is no __ex_table handling in address error hanlder (yet). > Does this mean it also traps for kernel memory accesses, or do those > work again? it will trap for every access. > If the addresses on mips64 are separate like on > sparc64 or s390, the entire access_ok() step could be replaced > by a fixup code in the exception handler. I suppose this depends on > CONFIG_EVA and you still need a limit check at least when EVA is > disabled. only EVA has seperate address spaces for kernel/user. > > Is there a reason to not also #define TASK_SIZE_MAX __UA_LIMIT like > > for the 32bit case ? > > > > For 32-bit, the __UA_LIMIT is a compile-time constant, so the check > ends up being trivial. On all other architectures, the same thing can > be done after the set_fs removal, so I was hoping it would work here > as well. ic > I suspect doing the generic (size <= limit) && (addr <= (limit - size)) > check on mips64 with the runtime limit ends up slightly slower > than the current code that checks a bit mask instead. If you like, > I'll update it this way, otherwise I'd need help in form of a patch > that changes the exception handling so __get_user/__put_user > also return -EFAULT for an address error. that's what the patch does. For aligned accesses the patch should do the right thing, but it breaks unaligned get_user/put_user. Checking if the trapping vaddr is between end of CPU VM space and TASK_MAX_SIZE before exception handling should do the trick. I'll send a patch, if this works. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea.[ RFC1925, 2.3 ] ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Mon, Feb 21, 2022 at 2:24 PM Thomas Bogendoerfer
wrote:
> On Wed, Feb 16, 2022 at 02:13:23PM +0100, Arnd Bergmann wrote:
> >
> > diff --git a/arch/mips/include/asm/uaccess.h
> > b/arch/mips/include/asm/uaccess.h
> > index db9a8e002b62..d7c89dc3426c 100644
>
> this doesn't work. For every access above maximum implemented virtual address
> space of the CPU an address error will be issued, but not a TLB miss.
> And address error isn't able to handle this situation.
Ah, so the __ex_table entry only catches TLB misses?
Does this mean it also traps for kernel memory accesses, or do those
work again? If the addresses on mips64 are separate like on
sparc64 or s390, the entire access_ok() step could be replaced
by a fixup code in the exception handler. I suppose this depends on
CONFIG_EVA and you still need a limit check at least when EVA is
disabled.
> With this patch
>
> diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
> index df4b708c04a9..3911f1481f3d 100644
> --- a/arch/mips/kernel/unaligned.c
> +++ b/arch/mips/kernel/unaligned.c
> @@ -1480,6 +1480,13 @@ asmlinkage void do_ade(struct pt_regs *regs)
> prev_state = exception_enter();
> perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS,
> 1, regs, regs->cp0_badvaddr);
> +
> + /* Are we prepared to handle this kernel fault? */
> + if (fixup_exception(regs)) {
> + current->thread.cp0_baduaddr = regs->cp0_badvaddr;
> + return;
> + }
> +
> /*
> * Did we catch a fault trying to load an instruction?
> */
>
> I at least get my simple test cases fixed, but I'm not sure this is
> correct.
>
> Is there a reason to not also #define TASK_SIZE_MAX __UA_LIMIT like
> for the 32bit case ?
>
For 32-bit, the __UA_LIMIT is a compile-time constant, so the check
ends up being trivial. On all other architectures, the same thing can
be done after the set_fs removal, so I was hoping it would work here
as well.
I suspect doing the generic (size <= limit) && (addr <= (limit - size))
check on mips64 with the runtime limit ends up slightly slower
than the current code that checks a bit mask instead. If you like,
I'll update it this way, otherwise I'd need help in form of a patch
that changes the exception handling so __get_user/__put_user
also return -EFAULT for an address error.
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 09/18] mips: use simpler access_ok()
On Wed, Feb 16, 2022 at 02:13:23PM +0100, Arnd Bergmann wrote:
>
> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> index db9a8e002b62..d7c89dc3426c 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -19,6 +19,7 @@
> #ifdef CONFIG_32BIT
>
> #define __UA_LIMIT 0x8000UL
> +#define TASK_SIZE_MAX__UA_LIMIT
>
> #define __UA_ADDR".word"
> #define __UA_LA "la"
> @@ -33,6 +34,7 @@
> extern u64 __ua_limit;
>
> #define __UA_LIMIT __ua_limit
> +#define TASK_SIZE_MAXXKSSEG
this doesn't work. For every access above maximum implemented virtual address
space of the CPU an address error will be issued, but not a TLB miss.
And address error isn't able to handle this situation.
With this patch
diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
index df4b708c04a9..3911f1481f3d 100644
--- a/arch/mips/kernel/unaligned.c
+++ b/arch/mips/kernel/unaligned.c
@@ -1480,6 +1480,13 @@ asmlinkage void do_ade(struct pt_regs *regs)
prev_state = exception_enter();
perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS,
1, regs, regs->cp0_badvaddr);
+
+ /* Are we prepared to handle this kernel fault? */
+ if (fixup_exception(regs)) {
+ current->thread.cp0_baduaddr = regs->cp0_badvaddr;
+ return;
+ }
+
/*
* Did we catch a fault trying to load an instruction?
*/
I at least get my simple test cases fixed, but I'm not sure this is
correct.
Is there a reason to not also #define TASK_SIZE_MAX __UA_LIMIT like
for the 32bit case ?
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.[ RFC1925, 2.3 ]
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
