| Message ID | 1398103808-24380-1-git-send-email-rabin@rab.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 21, 2014 at 07:10:08PM +0100, Rabin Vincent wrote: > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > strncpy_from_user functions") apparently broken those string operations > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > restricts the addresses to TASK_SIZE. > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > restrict anything. Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail. Anyway, the code looks fine to me: Acked-by: Will Deacon <will.deacon@arm.com> Will
On Tue, Apr 22, 2014 at 10:44:24AM +0100, Will Deacon wrote: > On Mon, Apr 21, 2014 at 07:10:08PM +0100, Rabin Vincent wrote: > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > strncpy_from_user functions") apparently broken those string operations > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > restricts the addresses to TASK_SIZE. > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > restrict anything. > > Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 > (since it's a 32-bit quantity), so checks like addr < user_addr_max() will > fail. Thanks for the ack, but I don't quite understand what you mean here. You describe the state before this patch, right? Why does it matter that KERNEL_DS is 0x0?
Hi Rabin, On Thu, Apr 24, 2014 at 04:43:20PM +0100, Rabin Vincent wrote: > On Tue, Apr 22, 2014 at 10:44:24AM +0100, Will Deacon wrote: > > On Mon, Apr 21, 2014 at 07:10:08PM +0100, Rabin Vincent wrote: > > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > > strncpy_from_user functions") apparently broken those string operations > > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > > restricts the addresses to TASK_SIZE. > > > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > > restrict anything. > > > > Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 > > (since it's a 32-bit quantity), so checks like addr < user_addr_max() will > > fail. > > Thanks for the ack, but I don't quite understand what you mean here. > You describe the state before this patch, right? Why does it matter > that KERNEL_DS is 0x0? Apologies, I misread the code that you're patching so I guess I'll have to revoke my ack, sorry! That's not to say I think the patch is bad, I'd just like to discuss it with you a bit more. Having re-read the code, the issue is because TASK_SIZE is defined as CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). What we actually want for !MMU is END_MEM instead of TASK_SIZE. Will
On Fri, Apr 25, 2014 at 10:12:24AM +0100, Will Deacon wrote: > On Thu, Apr 24, 2014 at 04:43:20PM +0100, Rabin Vincent wrote: > > On Tue, Apr 22, 2014 at 10:44:24AM +0100, Will Deacon wrote: > > > On Mon, Apr 21, 2014 at 07:10:08PM +0100, Rabin Vincent wrote: > > > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > > > strncpy_from_user functions") apparently broken those string operations > > > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > > > restricts the addresses to TASK_SIZE. > > > > > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > > > restrict anything. > > > > > > Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 > > > (since it's a 32-bit quantity), so checks like addr < user_addr_max() will > > > fail. > > > > Thanks for the ack, but I don't quite understand what you mean here. > > You describe the state before this patch, right? Why does it matter > > that KERNEL_DS is 0x0? > > Apologies, I misread the code that you're patching so I guess I'll have to > revoke my ack, sorry! That's not to say I think the patch is bad, I'd just > like to discuss it with you a bit more. > > Having re-read the code, the issue is because TASK_SIZE is defined as > CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means > TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). > What we actually want for !MMU is END_MEM instead of TASK_SIZE. I guess you mean I should #define user_addr_max() to END_MEM for !MMU? That won't work. For example, on a !MMU boot, the first thing that fails because of this bug is devtmpsfs_mount() -> sys_mount() -> copy_mount_string() -> strndup_user(), which is run in a kernel thread. The type argument of sys_mount() points to a string in flash since we run an XIP kernel. This flash address has no relation to END_MEM (which is nothing but CONFIG_DRAM_BASE + CONFIG_DRAM_SIZE) and could be higher than END_MEM.
On Fri, Apr 25, 2014 at 07:45:10PM +0100, Rabin Vincent wrote: > On Fri, Apr 25, 2014 at 10:12:24AM +0100, Will Deacon wrote: > > On Thu, Apr 24, 2014 at 04:43:20PM +0100, Rabin Vincent wrote: > > > On Tue, Apr 22, 2014 at 10:44:24AM +0100, Will Deacon wrote: > > > > On Mon, Apr 21, 2014 at 07:10:08PM +0100, Rabin Vincent wrote: > > > > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > > > > strncpy_from_user functions") apparently broken those string operations > > > > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > > > > restricts the addresses to TASK_SIZE. > > > > > > > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > > > > restrict anything. > > > > > > > > Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 > > > > (since it's a 32-bit quantity), so checks like addr < user_addr_max() will > > > > fail. > > > > > > Thanks for the ack, but I don't quite understand what you mean here. > > > You describe the state before this patch, right? Why does it matter > > > that KERNEL_DS is 0x0? > > > > Apologies, I misread the code that you're patching so I guess I'll have to > > revoke my ack, sorry! That's not to say I think the patch is bad, I'd just > > like to discuss it with you a bit more. > > > > Having re-read the code, the issue is because TASK_SIZE is defined as > > CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means > > TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). > > What we actually want for !MMU is END_MEM instead of TASK_SIZE. > > I guess you mean I should #define user_addr_max() to END_MEM for !MMU? > That won't work. For example, on a !MMU boot, the first thing that > fails because of this bug is devtmpsfs_mount() -> sys_mount() -> > copy_mount_string() -> strndup_user(), which is run in a kernel thread. > The type argument of sys_mount() points to a string in flash since we > run an XIP kernel. This flash address has no relation to END_MEM (which > is nothing but CONFIG_DRAM_BASE + CONFIG_DRAM_SIZE) and could be higher > than END_MEM. Good point, I hadn't considered XIP with flash regions. In which case, I don't think we have any choice but to use ~0UL as the limit. It looks like Uwe has taken a quick look at some other users of TASK_SIZE, so I'll leave you two to complete the audit ;) Will
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h index 12c3a5d..c3a65f1 100644 --- a/arch/arm/include/asm/uaccess.h +++ b/arch/arm/include/asm/uaccess.h @@ -199,6 +199,9 @@ extern int __put_user_8(void *, unsigned long long); __put_user_check(x,p); \ }) +#define user_addr_max() \ + (segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL) + #else /* CONFIG_MMU */ /* @@ -210,6 +213,7 @@ extern int __put_user_8(void *, unsigned long long); #define __addr_ok(addr) ((void)(addr),1) #define __range_ok(addr,size) ((void)(addr),0) #define get_fs() (KERNEL_DS) +#define user_addr_max() (~0UL) static inline void set_fs(mm_segment_t fs) { @@ -222,9 +226,6 @@ static inline void set_fs(mm_segment_t fs) #define access_ok(type,addr,size) (__range_ok(addr,size) == 0) -#define user_addr_max() \ - (segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL) - /* * The "__xxx" versions of the user access functions do not verify the * address space - it must have been done previously with a separate
8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE. TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything. Signed-off-by: Rabin Vincent <rabin@rab.in> --- arch/arm/include/asm/uaccess.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)