| Message ID | 1618139092-4018-1-git-send-email-hejinyang@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Fix strnlen_user access check | expand |
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
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
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
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__(
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.
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
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)
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.
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)
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.
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)
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.
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__(
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(-)