Message ID | 20220216131332.1489939-10-arnd@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | clean up asm/uaccess.h, kill set_fs for good | expand |
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 0x80000000UL > +#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_MAX XKSSEG 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.
On Mon, Feb 21, 2022 at 2:24 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> 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
On Mon, Feb 21, 2022 at 03:31:23PM +0100, Arnd Bergmann wrote: > On Mon, Feb 21, 2022 at 2:24 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> 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.
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-tsbogend@alpha.franken.de/ That does the trick. Thomas.
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 0x80000000UL > +#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.
On Wed, Feb 23, 2022 at 8:41 AM Thomas Bogendoerfer <tsbogend@alpha.franken.de> 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 0x80000000UL > > +#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
On Mon, Feb 21, 2022 at 5:25 AM Thomas Bogendoerfer <tsbogend@alpha.franken.de> 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
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 0x80000000UL +#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_MAX XKSSEG #define __UA_ADDR ".dword" #define __UA_LA "dla" @@ -42,22 +44,6 @@ extern u64 __ua_limit; #endif /* CONFIG_64BIT */ -/* - * Is a address valid? This does a straightforward calculation rather - * than tests. - * - * Address valid if: - * - "addr" doesn't have any high-bits set - * - AND "size" doesn't have any high-bits set - * - AND "addr+size" doesn't have any high-bits set - * - OR we are in kernel mode. - * - * __ua_size() is a trick to avoid runtime checking of positive constant - * sizes; for those we already know at compile time that the size is ok. - */ -#define __ua_size(size) \ - ((__builtin_constant_p(size) && (signed long) (size) > 0) ? 0 : (size)) - /* * access_ok: - Checks if a user space pointer is valid * @addr: User space pointer to start of block to check @@ -79,9 +65,9 @@ extern u64 __ua_limit; static inline int __access_ok(const void __user *p, unsigned long size) { unsigned long addr = (unsigned long)p; - unsigned long end = addr + size - !!size; + unsigned long limit = TASK_SIZE_MAX; - return (__UA_LIMIT & (addr | end | __ua_size(size))) == 0; + return (size <= limit) && (addr <= (limit - size)); } #define access_ok(addr, size) \