* [PATCH] MIPS: Fix strnlen_user access check @ 2021-04-11 11:04 Jinyang He 2021-04-12 3:02 ` Tiezhu Yang 2021-04-12 13:47 ` Jinyang He 0 siblings, 2 replies; 15+ messages in thread From: Jinyang He @ 2021-04-11 11:04 UTC (permalink / raw) To: Thomas Bogendoerfer; +Cc: linux-mips, linux-kernel Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for strnlen_user(). Jump out when checking access_ok() with condition that (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() just checked (ua_limit & s) without checking (ua_limit & (s + n)). Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. Signed-off-by: Jinyang He <hejinyang@loongson.cn> --- arch/mips/include/asm/uaccess.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h index 91bc7fb..85ba0c8 100644 --- a/arch/mips/include/asm/uaccess.h +++ b/arch/mips/include/asm/uaccess.h @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) { long res; - if (!access_ok(s, n)) - return -0; + if (unlikely(n <= 0)) + return 0; + + if (!access_ok(s, n)) { + if (!access_ok(s, 0)) + return 0; + + n = __UA_LIMIT - (unsigned long)s - 1; + } might_fault(); __asm__ __volatile__( -- 2.1.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-11 11:04 [PATCH] MIPS: Fix strnlen_user access check Jinyang He @ 2021-04-12 3:02 ` Tiezhu Yang 2021-04-12 6:06 ` Jinyang He ` (2 more replies) 2021-04-12 13:47 ` Jinyang He 1 sibling, 3 replies; 15+ messages in thread From: Tiezhu Yang @ 2021-04-12 3:02 UTC (permalink / raw) To: Jinyang He, Thomas Bogendoerfer; +Cc: linux-mips, linux-kernel On 04/11/2021 07:04 PM, Jinyang He wrote: > Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for > strnlen_user(). Jump out when checking access_ok() with condition that > (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() > just checked (ua_limit & s) without checking (ua_limit & (s + n)). > Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. > > Signed-off-by: Jinyang He <hejinyang@loongson.cn> > --- > arch/mips/include/asm/uaccess.h | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > index 91bc7fb..85ba0c8 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) > { > long res; > > - if (!access_ok(s, n)) > - return -0; > + if (unlikely(n <= 0)) > + return 0; > + > + if (!access_ok(s, n)) { > + if (!access_ok(s, 0)) > + return 0; > + > + n = __UA_LIMIT - (unsigned long)s - 1; > + } > > might_fault(); > __asm__ __volatile__( The following simple changes are OK to fix this issue? diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h index 91bc7fb..eafc99b 100644 --- a/arch/mips/include/asm/uaccess.h +++ b/arch/mips/include/asm/uaccess.h @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n) { long res; - if (!access_ok(s, n)) - return -0; + if (!access_ok(s, 1)) + return 0; might_fault(); __asm__ __volatile__( Thanks, Tiezhu ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-12 3:02 ` Tiezhu Yang @ 2021-04-12 6:06 ` Jinyang He 2021-04-12 7:08 ` Tiezhu Yang 2021-04-12 14:27 ` Thomas Bogendoerfer 2 siblings, 0 replies; 15+ messages in thread From: Jinyang He @ 2021-04-12 6:06 UTC (permalink / raw) To: Tiezhu Yang, Thomas Bogendoerfer; +Cc: linux-mips, linux-kernel On 04/12/2021 11:02 AM, Tiezhu Yang wrote: > On 04/11/2021 07:04 PM, Jinyang He wrote: >> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for >> strnlen_user(). Jump out when checking access_ok() with condition that >> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() >> just checked (ua_limit & s) without checking (ua_limit & (s + n)). >> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. >> >> Signed-off-by: Jinyang He <hejinyang@loongson.cn> >> --- >> arch/mips/include/asm/uaccess.h | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/arch/mips/include/asm/uaccess.h >> b/arch/mips/include/asm/uaccess.h >> index 91bc7fb..85ba0c8 100644 >> --- a/arch/mips/include/asm/uaccess.h >> +++ b/arch/mips/include/asm/uaccess.h >> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char >> __user *s, long n) >> { >> long res; >> - if (!access_ok(s, n)) >> - return -0; >> + if (unlikely(n <= 0)) >> + return 0; >> + >> + if (!access_ok(s, n)) { >> + if (!access_ok(s, 0)) >> + return 0; >> + >> + n = __UA_LIMIT - (unsigned long)s - 1; >> + } >> might_fault(); >> __asm__ __volatile__( > > The following simple changes are OK to fix this issue? > > diff --git a/arch/mips/include/asm/uaccess.h > b/arch/mips/include/asm/uaccess.h > index 91bc7fb..eafc99b 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user > *s, long n) > { > long res; > > - if (!access_ok(s, n)) > - return -0; > + if (!access_ok(s, 1)) > + return 0; > > might_fault(); > __asm__ __volatile__( > > Thanks, > Tiezhu > Thanks for your comment. That looks similar to other archs, but I don't know how the access_ok() implementation in other archs. Using access_ok(s, 0) is similar to the old strnlen_user(). Using access_ok(s, 1) may have a problem in this extreme case, s = __UA_LIMIT - 1, *s = 0, and we hope it returns 1. But it returns 0 by !access_ok(s, 1). Of course, it is so extrme. More importantly, I want to set up a maximum for strnlen_user_asm. And do not access the part of beyond __ua_limit. As follow shows, +-----------+ | ... | +-----------+ <---- s + n | 0 | +-----------+ | s | +-----------+ | r | +-----------+ | e | +-----------+ | h | +-----------+ | t | +-----------+ | o | +-----------+ <---- __UA_LIMIT | r | +-----------+ | t | +-----------+ | s | +-----------+ <---- s | ... | +-----------+ It is dangerous to access "others", for user, only "str" is safe. I don't know whether it would be happend, I just limited it by change `n`. Should do other things if meet __UA_LIMIT - 1? Thanks, Jinyang ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-12 3:02 ` Tiezhu Yang 2021-04-12 6:06 ` Jinyang He @ 2021-04-12 7:08 ` Tiezhu Yang 2021-04-12 14:27 ` Thomas Bogendoerfer 2 siblings, 0 replies; 15+ messages in thread From: Tiezhu Yang @ 2021-04-12 7:08 UTC (permalink / raw) To: Jinyang He, Thomas Bogendoerfer; +Cc: linux-mips, linux-kernel On 04/12/2021 11:02 AM, Tiezhu Yang wrote: > On 04/11/2021 07:04 PM, Jinyang He wrote: >> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for >> strnlen_user(). Jump out when checking access_ok() with condition that >> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() >> just checked (ua_limit & s) without checking (ua_limit & (s + n)). >> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. >> >> Signed-off-by: Jinyang He <hejinyang@loongson.cn> >> --- >> arch/mips/include/asm/uaccess.h | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/arch/mips/include/asm/uaccess.h >> b/arch/mips/include/asm/uaccess.h >> index 91bc7fb..85ba0c8 100644 >> --- a/arch/mips/include/asm/uaccess.h >> +++ b/arch/mips/include/asm/uaccess.h >> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char >> __user *s, long n) >> { >> long res; >> - if (!access_ok(s, n)) >> - return -0; >> + if (unlikely(n <= 0)) >> + return 0; >> + >> + if (!access_ok(s, n)) { >> + if (!access_ok(s, 0)) >> + return 0; >> + >> + n = __UA_LIMIT - (unsigned long)s - 1; >> + } >> might_fault(); >> __asm__ __volatile__( > > The following simple changes are OK to fix this issue? > > diff --git a/arch/mips/include/asm/uaccess.h > b/arch/mips/include/asm/uaccess.h > index 91bc7fb..eafc99b 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user > *s, long n) > { > long res; > > - if (!access_ok(s, n)) > - return -0; > + if (!access_ok(s, 1)) > + return 0; > > might_fault(); > __asm__ __volatile__( > > Thanks, > Tiezhu > Hi all, Here is some detail info about background and analysis process, I hope it is useful to understand this issue. When update kernel with the latest mips-next, we can not login through a graphical interface, this is because drm radeon GPU driver does not work, we can not see the boot message "[drm] radeon kernel modesetting enabled." through the serial console. drivers/gpu/drm/radeon/radeon_drv.c static int __init radeon_module_init(void) { [...] DRM_INFO("radeon kernel modesetting enabled.\n"); [...] } I use git bisect to find commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") is the first bad commit: $ git bisect log git bisect start # good: [666c1fc90cd82184624d4cc5d124c66025f89a47] mips: bmips: bcm63268: populate device tree nodes git bisect good 666c1fc90cd82184624d4cc5d124c66025f89a47 # bad: [e86e75596623e1ce5d784db8214687326712a8ae] MIPS: octeon: Add __raw_copy_[from|to|in]_user symbols git bisect bad e86e75596623e1ce5d784db8214687326712a8ae # good: [45deb5faeb9e02951361ceba5ffee721745661c3] MIPS: uaccess: Remove get_fs/set_fs call sites git bisect good 45deb5faeb9e02951361ceba5ffee721745661c3 # bad: [5e65c52ec716af6e8f51dacdaeb4a4d872249af1] MIPS: Loongson64: Use _CACHE_UNCACHED instead of _CACHE_UNCACHED_ACCELERATED git bisect bad 5e65c52ec716af6e8f51dacdaeb4a4d872249af1 # bad: [04324f44cb69a03fdc8f2ee52386a4fdf6a0043b] MIPS: Remove get_fs/set_fs git bisect bad 04324f44cb69a03fdc8f2ee52386a4fdf6a0043b # first bad commit: [04324f44cb69a03fdc8f2ee52386a4fdf6a0043b] MIPS: Remove get_fs/set_fs I analysis and test the changes in the above first bad commit and find out the following obvious difference which leads to the login issue. arch/mips/include/asm/uaccess.h static inline long strnlen_user(const char __user *s, long n) { [...] if (!access_ok(s, n)) return -0; [...] } Thanks, Tiezhu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-12 3:02 ` Tiezhu Yang 2021-04-12 6:06 ` Jinyang He 2021-04-12 7:08 ` Tiezhu Yang @ 2021-04-12 14:27 ` Thomas Bogendoerfer 2021-04-13 1:15 ` Jinyang He 2 siblings, 1 reply; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-12 14:27 UTC (permalink / raw) To: Tiezhu Yang; +Cc: Jinyang He, linux-mips, linux-kernel On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote: > On 04/11/2021 07:04 PM, Jinyang He wrote: > > Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for > > strnlen_user(). Jump out when checking access_ok() with condition that > > (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() > > just checked (ua_limit & s) without checking (ua_limit & (s + n)). > > Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. > > > > Signed-off-by: Jinyang He <hejinyang@loongson.cn> > > --- > > arch/mips/include/asm/uaccess.h | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > > index 91bc7fb..85ba0c8 100644 > > --- a/arch/mips/include/asm/uaccess.h > > +++ b/arch/mips/include/asm/uaccess.h > > @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) > > { > > long res; > > - if (!access_ok(s, n)) > > - return -0; > > + if (unlikely(n <= 0)) > > + return 0; > > + > > + if (!access_ok(s, n)) { > > + if (!access_ok(s, 0)) > > + return 0; > > + > > + n = __UA_LIMIT - (unsigned long)s - 1; > > + } > > might_fault(); > > __asm__ __volatile__( > > The following simple changes are OK to fix this issue? > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > index 91bc7fb..eafc99b 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n) > { > long res; > - if (!access_ok(s, n)) > - return -0; > + if (!access_ok(s, 1)) > + return 0; > might_fault(); > __asm__ __volatile__( that's the fix I'd like to apply. Could someone send it as a formal patch ? Thanks. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-12 14:27 ` Thomas Bogendoerfer @ 2021-04-13 1:15 ` Jinyang He 2021-04-13 8:34 ` David Laight 2021-04-13 11:14 ` Thomas Bogendoerfer 0 siblings, 2 replies; 15+ messages in thread From: Jinyang He @ 2021-04-13 1:15 UTC (permalink / raw) To: Thomas Bogendoerfer, Tiezhu Yang; +Cc: linux-mips, linux-kernel On 04/12/2021 10:27 PM, Thomas Bogendoerfer wrote: > On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote: >> On 04/11/2021 07:04 PM, Jinyang He wrote: >>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for >>> strnlen_user(). Jump out when checking access_ok() with condition that >>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() >>> just checked (ua_limit & s) without checking (ua_limit & (s + n)). >>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. >>> >>> Signed-off-by: Jinyang He <hejinyang@loongson.cn> >>> --- >>> arch/mips/include/asm/uaccess.h | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h >>> index 91bc7fb..85ba0c8 100644 >>> --- a/arch/mips/include/asm/uaccess.h >>> +++ b/arch/mips/include/asm/uaccess.h >>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) >>> { >>> long res; >>> - if (!access_ok(s, n)) >>> - return -0; >>> + if (unlikely(n <= 0)) >>> + return 0; >>> + >>> + if (!access_ok(s, n)) { >>> + if (!access_ok(s, 0)) >>> + return 0; >>> + >>> + n = __UA_LIMIT - (unsigned long)s - 1; >>> + } >>> might_fault(); >>> __asm__ __volatile__( >> The following simple changes are OK to fix this issue? >> >> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h >> index 91bc7fb..eafc99b 100644 >> --- a/arch/mips/include/asm/uaccess.h >> +++ b/arch/mips/include/asm/uaccess.h >> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n) >> { >> long res; >> - if (!access_ok(s, n)) >> - return -0; >> + if (!access_ok(s, 1)) >> + return 0; >> might_fault(); >> __asm__ __volatile__( > that's the fix I'd like to apply. Could someone send it as a formal > patch ? Thanks. > > Thomas. > Hi, Thomas, Thank you for bringing me more thinking. I always think it is better to use access_ok(s, 0) on MIPS. I have been curious about the difference between access_ok(s, 0) and access_ok(s, 1) until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h The __access_ok() is noted with `Ensure that the range [addr, addr+size) is within the process's address space`. Does the range checked by __access_ok() on MIPS is [addr, addr+size]. So if we want to use access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? More importantly, the implementation of strnlen_user in lib/strnlen_user.c is noted `we hit the address space limit, and we still had more characters the caller would have wanted. That's 0.` Does it make sense? It is not achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used. Thanks, Jinyang ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 1:15 ` Jinyang He @ 2021-04-13 8:34 ` David Laight 2021-04-13 11:14 ` Thomas Bogendoerfer 1 sibling, 0 replies; 15+ messages in thread From: David Laight @ 2021-04-13 8:34 UTC (permalink / raw) To: 'Jinyang He', Thomas Bogendoerfer, Tiezhu Yang Cc: linux-mips, linux-kernel From: Jinyang He > Sent: 13 April 2021 02:16 > > > On Mon, Apr 12, 2021 at 11:02:19AM +0800, Tiezhu Yang wrote: > >> On 04/11/2021 07:04 PM, Jinyang He wrote: > >>> Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for > >>> strnlen_user(). Jump out when checking access_ok() with condition that > >>> (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() > >>> just checked (ua_limit & s) without checking (ua_limit & (s + n)). > >>> Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. > >>> > >>> Signed-off-by: Jinyang He <hejinyang@loongson.cn> > >>> --- > >>> arch/mips/include/asm/uaccess.h | 11 +++++++++-- > >>> 1 file changed, 9 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > >>> index 91bc7fb..85ba0c8 100644 > >>> --- a/arch/mips/include/asm/uaccess.h > >>> +++ b/arch/mips/include/asm/uaccess.h > >>> @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) > >>> { > >>> long res; > >>> - if (!access_ok(s, n)) > >>> - return -0; > >>> + if (unlikely(n <= 0)) > >>> + return 0; > >>> + > >>> + if (!access_ok(s, n)) { > >>> + if (!access_ok(s, 0)) > >>> + return 0; > >>> + > >>> + n = __UA_LIMIT - (unsigned long)s - 1; > >>> + } > >>> might_fault(); > >>> __asm__ __volatile__( > >> The following simple changes are OK to fix this issue? > >> > >> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > >> index 91bc7fb..eafc99b 100644 > >> --- a/arch/mips/include/asm/uaccess.h > >> +++ b/arch/mips/include/asm/uaccess.h > >> @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n) > >> { > >> long res; > >> - if (!access_ok(s, n)) > >> - return -0; > >> + if (!access_ok(s, 1)) > >> + return 0; > >> might_fault(); > >> __asm__ __volatile__( > > that's the fix I'd like to apply. Could someone send it as a formal > > patch ? Thanks. > > > > Thomas. > > > Hi, Thomas, > > Thank you for bringing me more thinking. > > I always think it is better to use access_ok(s, 0) on MIPS. I have been > curious about the difference between access_ok(s, 0) and access_ok(s, 1) > until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > is within the process's address space`. Does the range checked by > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? ISTR that access_ok(xxx, 0) is unconditionally true on some architectures. The range checked should be [addr, addr+size). These are needed so that write(fd, random(), 0) doesn't ever fault. > More importantly, the implementation of strnlen_user in lib/strnlen_user.c > is noted `we hit the address space limit, and we still had more characters > the caller would have wanted. That's 0.` Does it make sense? It is not > achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used. There is the question of whether one call to access_ok(addr, 1) is sufficient for any code that does sequential accesses. It is if there is an unmapped page between the last valid user page and the first valid kernel page. IIRC x86 has such an unmapped page because 'horrid things' (tm) happen if the cpu prefetches across the user-kernel boundary. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 1:15 ` Jinyang He 2021-04-13 8:34 ` David Laight @ 2021-04-13 11:14 ` Thomas Bogendoerfer 2021-04-13 12:37 ` David Laight 1 sibling, 1 reply; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-13 11:14 UTC (permalink / raw) To: Jinyang He; +Cc: Tiezhu Yang, linux-mips, linux-kernel On Tue, Apr 13, 2021 at 09:15:48AM +0800, Jinyang He wrote: > On 04/12/2021 10:27 PM, Thomas Bogendoerfer wrote: > > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > > > index 91bc7fb..eafc99b 100644 > > > --- a/arch/mips/include/asm/uaccess.h > > > +++ b/arch/mips/include/asm/uaccess.h > > > @@ -630,8 +630,8 @@ static inline long strnlen_user(const char __user *s, long n) > > > { > > > long res; > > > - if (!access_ok(s, n)) > > > - return -0; > > > + if (!access_ok(s, 1)) > > > + return 0; > > > might_fault(); > > > __asm__ __volatile__( > > that's the fix I'd like to apply. Could someone send it as a formal > > patch ? Thanks. > > > > Thomas. > > > Hi, Thomas, > > I always think it is better to use access_ok(s, 0) on MIPS. I have been > curious about the difference between access_ok(s, 0) and access_ok(s, 1) > until I saw __access_ok() on RISCV at arch/riscv/include/asm/uaccess.h > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > is within the process's address space`. Does the range checked by > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? you are right, I'm going to apply https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/ to fix that. > More importantly, the implementation of strnlen_user in lib/strnlen_user.c > is noted `we hit the address space limit, and we still had more characters > the caller would have wanted. That's 0.` Does it make sense? It is not > achieved on MIPS when hit __ua_limit, if only access_ok(s, 1) is used. see the comment in arch/mips/lib/strnlen_user.S * Note: for performance reasons we deliberately accept that a user may * make strlen_user and strnlen_user access the first few KSEG0 * bytes. There's nothing secret there. On 64-bit accessing beyond * the maximum is a tad hairier ... for 32bit kernels strnlen_user could possibly access KSEG0 and will find a 0 sooner or later. I don't see much problems there. For 64bit kernels strnlen_user will stop inside user space as there will be nothing mapped after __UA_LIMIT. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 11:14 ` Thomas Bogendoerfer @ 2021-04-13 12:37 ` David Laight 2021-04-13 15:19 ` Thomas Bogendoerfer 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2021-04-13 12:37 UTC (permalink / raw) To: 'Thomas Bogendoerfer', Jinyang He Cc: Tiezhu Yang, linux-mips, linux-kernel From: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Sent: 13 April 2021 12:15 ... > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > > is within the process's address space`. Does the range checked by > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? > > you are right, I'm going to apply > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/ > > to fix that. Isn't that still wrong? If an application does: write(fd, (void *)0xffff0000, 0); it should return 0, not -1 and EFAULT/SIGSEGV. There is also the question about why this makes any difference to the original problem of logging in via the graphical interface. ISTM that it is very unlikely that the length passed to strnlen_user() is long enough to take potential buffer beyond the end of user address space. It might be that it is passing 'huge' to do strlen_user(). But since the remove set_fs() changes are reported to have broken it, was it actually being called for a kernel buffer? There is more going on here. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 12:37 ` David Laight @ 2021-04-13 15:19 ` Thomas Bogendoerfer 2021-04-13 16:01 ` David Laight 0 siblings, 1 reply; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-13 15:19 UTC (permalink / raw) To: David Laight; +Cc: Jinyang He, Tiezhu Yang, linux-mips, linux-kernel On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote: > From: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Sent: 13 April 2021 12:15 > ... > > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > > > is within the process's address space`. Does the range checked by > > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? > > > > you are right, I'm going to apply > > > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/ > > > > to fix that. > > Isn't that still wrong? > If an application does: > write(fd, (void *)0xffff0000, 0); > it should return 0, not -1 and EFAULT/SIGSEGV. WRITE(2) Linux Programmer's Manual WRITE(2) [...] If count is zero and fd refers to a regular file, then write() may return a failure status if one of the errors below is detected. If no errors are detected, or error detection is not performed, 0 will be returned without causing any other effect. If count is zero and fd refers to a file other than a regular file, the results are not speci- fied. [...] EFAULT buf is outside your accessible address space. at least it's covered by the man page on my Linux system. > There is also the question about why this makes any difference > to the original problem of logging in via the graphical interface. kernel/module.c: mod->args = strndup_user(uargs, ~0UL >> 1); and strndup_user does a strnlen_user. > ISTM that it is very unlikely that the length passed to strnlen_user() > is long enough to take potential buffer beyond the end of user > address space. see above. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 15:19 ` Thomas Bogendoerfer @ 2021-04-13 16:01 ` David Laight 2021-04-14 7:59 ` Thomas Bogendoerfer 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2021-04-13 16:01 UTC (permalink / raw) To: 'Thomas Bogendoerfer' Cc: Jinyang He, Tiezhu Yang, linux-mips, linux-kernel From: Thomas Bogendoerfer > Sent: 13 April 2021 16:19 > > On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote: > > From: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > Sent: 13 April 2021 12:15 > > ... > > > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > > > > is within the process's address space`. Does the range checked by > > > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > > > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? > > > > > > you are right, I'm going to apply > > > > > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/ > > > > > > to fix that. > > > > Isn't that still wrong? > > If an application does: > > write(fd, (void *)0xffff0000, 0); > > it should return 0, not -1 and EFAULT/SIGSEGV. > > WRITE(2) Linux Programmer's Manual WRITE(2) > [...] > If count is zero and fd refers to a regular file, then write() may > return a failure status if one of the errors below is detected. If no > errors are detected, or error detection is not performed, 0 will be > returned without causing any other effect. If count is zero and fd > refers to a file other than a regular file, the results are not speci- > fied. > [...] > EFAULT buf is outside your accessible address space. > > at least it's covered by the man page on my Linux system. Something related definitely caused grief in the setsockopt() changes. > > There is also the question about why this makes any difference > > to the original problem of logging in via the graphical interface. > > kernel/module.c: mod->args = strndup_user(uargs, ~0UL >> 1); > > and strndup_user does a strnlen_user. That call is just gross. Why did it work before the removal of set_fs() etc. Or was there another change that affected strndup_user() ? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-13 16:01 ` David Laight @ 2021-04-14 7:59 ` Thomas Bogendoerfer 0 siblings, 0 replies; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-14 7:59 UTC (permalink / raw) To: David Laight; +Cc: Jinyang He, Tiezhu Yang, linux-mips, linux-kernel On Tue, Apr 13, 2021 at 04:01:13PM +0000, David Laight wrote: > From: Thomas Bogendoerfer > > Sent: 13 April 2021 16:19 > > > > On Tue, Apr 13, 2021 at 12:37:25PM +0000, David Laight wrote: > > > From: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > > Sent: 13 April 2021 12:15 > > > ... > > > > > The __access_ok() is noted with `Ensure that the range [addr, addr+size) > > > > > is within the process's address space`. Does the range checked by > > > > > __access_ok() on MIPS is [addr, addr+size]. So if we want to use > > > > > access_ok(s, 1), should we modify __access_ok()? Or my misunderstanding? > > > > > > > > you are right, I'm going to apply > > > > > > > > https://patchwork.kernel.org/project/linux-mips/patch/20190209194718.1294-1-paul.burton@mips.com/ > > > > > > > > to fix that. > > > > > > Isn't that still wrong? > > > If an application does: > > > write(fd, (void *)0xffff0000, 0); > > > it should return 0, not -1 and EFAULT/SIGSEGV. > > > > WRITE(2) Linux Programmer's Manual WRITE(2) > > [...] > > If count is zero and fd refers to a regular file, then write() may > > return a failure status if one of the errors below is detected. If no > > errors are detected, or error detection is not performed, 0 will be > > returned without causing any other effect. If count is zero and fd > > refers to a file other than a regular file, the results are not speci- > > fied. > > [...] > > EFAULT buf is outside your accessible address space. > > > > at least it's covered by the man page on my Linux system. > > Something related definitely caused grief in the setsockopt() changes. > > > > There is also the question about why this makes any difference > > > to the original problem of logging in via the graphical interface. > > > > kernel/module.c: mod->args = strndup_user(uargs, ~0UL >> 1); > > > > and strndup_user does a strnlen_user. > > That call is just gross. > Why did it work before the removal of set_fs() etc. strnlen_user just did the equivalent of access_ok(s, 0) and I copy&pasted the wrong access_ok() statement :-( > Or was there another change that affected strndup_user() ? no, just the change in strnlen_user. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-11 11:04 [PATCH] MIPS: Fix strnlen_user access check Jinyang He 2021-04-12 3:02 ` Tiezhu Yang @ 2021-04-12 13:47 ` Jinyang He 1 sibling, 0 replies; 15+ messages in thread From: Jinyang He @ 2021-04-12 13:47 UTC (permalink / raw) To: Thomas Bogendoerfer; +Cc: linux-mips, linux-kernel On 04/11/2021 07:04 PM, Jinyang He wrote: > Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") brought a problem for > strnlen_user(). Jump out when checking access_ok() with condition that > (s + strlen(s)) < __UA_LIMIT <= (s + n). The old __strnlen_user_asm() > just checked (ua_limit & s) without checking (ua_limit & (s + n)). > Therefore, find strlen form s to __UA_LIMIT - 1 in that condition. > > Signed-off-by: Jinyang He <hejinyang@loongson.cn> > --- > arch/mips/include/asm/uaccess.h | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h > index 91bc7fb..85ba0c8 100644 > --- a/arch/mips/include/asm/uaccess.h > +++ b/arch/mips/include/asm/uaccess.h > @@ -630,8 +630,15 @@ static inline long strnlen_user(const char __user *s, long n) > { > long res; > > - if (!access_ok(s, n)) > - return -0; > + if (unlikely(n <= 0)) > + return 0; > + > + if (!access_ok(s, n)) { > + if (!access_ok(s, 0)) > + return 0; > + > + n = __UA_LIMIT - (unsigned long)s - 1; Sorry for here and it should be ~__UA_LIMIT... earlier discussed: https://patchwork.kernel.org/project/linux-mips/patch/1618139092-4018-1-git-send-email-hejinyang@loongson.cn/#24107107 Hope other comment. :-) Jinyang > + } > > might_fault(); > __asm__ __volatile__( ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] MIPS: Fix strnlen_user access check @ 2021-04-15 21:26 Thomas Bogendoerfer 2021-04-16 7:22 ` Thomas Bogendoerfer 0 siblings, 1 reply; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-15 21:26 UTC (permalink / raw) To: linux-mips, linux-kernel Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") changed the access_ok for strnlen_user to check the whole range, which broke some callers of strndup_user(). Restore the old behaviour and just check the first byte. Fixes: 04324f44cb69 ("MIPS: Remove get_fs/set_fs") Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> --- arch/mips/include/asm/uaccess.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h index ab47e597656a..783fecce65c8 100644 --- a/arch/mips/include/asm/uaccess.h +++ b/arch/mips/include/asm/uaccess.h @@ -614,8 +614,8 @@ static inline long strnlen_user(const char __user *s, long n) { long res; - if (!access_ok(s, n)) - return -0; + if (!access_ok(s, 1)) + return 0; might_fault(); __asm__ __volatile__( -- 2.29.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Fix strnlen_user access check 2021-04-15 21:26 Thomas Bogendoerfer @ 2021-04-16 7:22 ` Thomas Bogendoerfer 0 siblings, 0 replies; 15+ messages in thread From: Thomas Bogendoerfer @ 2021-04-16 7:22 UTC (permalink / raw) To: linux-mips, linux-kernel On Thu, Apr 15, 2021 at 11:26:40PM +0200, Thomas Bogendoerfer wrote: > Commit 04324f44cb69 ("MIPS: Remove get_fs/set_fs") changed the access_ok > for strnlen_user to check the whole range, which broke some callers > of strndup_user(). Restore the old behaviour and just check the first byte. > > Fixes: 04324f44cb69 ("MIPS: Remove get_fs/set_fs") > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > --- > arch/mips/include/asm/uaccess.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) applied to mips-next. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-04-16 7:23 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-11 11:04 [PATCH] MIPS: Fix strnlen_user access check Jinyang He 2021-04-12 3:02 ` Tiezhu Yang 2021-04-12 6:06 ` Jinyang He 2021-04-12 7:08 ` Tiezhu Yang 2021-04-12 14:27 ` Thomas Bogendoerfer 2021-04-13 1:15 ` Jinyang He 2021-04-13 8:34 ` David Laight 2021-04-13 11:14 ` Thomas Bogendoerfer 2021-04-13 12:37 ` David Laight 2021-04-13 15:19 ` Thomas Bogendoerfer 2021-04-13 16:01 ` David Laight 2021-04-14 7:59 ` Thomas Bogendoerfer 2021-04-12 13:47 ` Jinyang He 2021-04-15 21:26 Thomas Bogendoerfer 2021-04-16 7:22 ` Thomas Bogendoerfer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).