[PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Yangxi Xiang]

a common calling pattern is strncpy_from_user(buf, user_ptr, sizeof(buf)).
However a buffer-overflow read occurs in this loop when reading the last
byte. Fix it by early checking the available bytes.

Signed-off-by: Yangxi Xiang <xyangxi5@gmail.com>
---
 include/asm-generic/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index a0b2f270dddc..d45d4f535934 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -252,7 +252,7 @@ __strncpy_from_user(char *dst, const char __user *src, long count)
 {
 	char *tmp;
 	strncpy(dst, (const char __force *)src, count);
-	for (tmp = dst; *tmp && count > 0; tmp++, count--)
+	for (tmp = dst; count > 0 && *tmp; tmp++, count--)
 		;
 	return (tmp - dst);
 }
-- 
2.17.1

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Arnd Bergmann]

On Mon, Jun 27, 2022 at 8:32 AM Yangxi Xiang <xyangxi5@gmail.com> wrote:
>
> a common calling pattern is strncpy_from_user(buf, user_ptr, sizeof(buf)).
> However a buffer-overflow read occurs in this loop when reading the last
> byte. Fix it by early checking the available bytes.
>
> Signed-off-by: Yangxi Xiang <xyangxi5@gmail.com>
> ---

This function was removed in commit 98b861a30431 ("asm-generic: uaccess:
remove inline strncpy_from_user/strnlen_user"), and the new version in
lib/strncpy_from_user.c does not have the problem

On which architecture and kernel version do you see the problem?

      Arnd

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Yangxi Xiang]

I also noticed that it was removed in commit 98b861a30431. I did see
this problem in kernel 5.1 and this problem remains in
architectures without selecting config GENERIC_STRNCPY_FROM_USER.

Yangxi Xiang

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Arnd Bergmann]

On Mon, Jun 27, 2022 at 9:40 AM Yangxi Xiang <xyangxi5@gmail.com> wrote:
>
> I also noticed that it was removed in commit 98b861a30431. I did see
> this problem in kernel 5.1 and this problem remains in
> architectures without selecting config GENERIC_STRNCPY_FROM_USER.

Which architectures do you mean? I don't see any architecture using
asm-generic/uaccess.h without setting GENERIC_STRNCPY_FROM_USER
before commit 98b861a30431 or the prior release.

Also, I think the implementation relied on strncpy() setting a zero pad
at the end of the string, so the ckeck would only be needed for a count
value that starts out negative? Is there another way this can actually
cause problems?

        Arnd

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Yangxi Xiang]

> Which architectures do you mean? I don't see any architecture using
> asm-generic/uaccess.h without setting GENERIC_STRNCPY_FROM_USER
> before commit 98b861a30431 or the prior release.

I am a user of LibOS, which uses this __strncpy_from_user.

> Also, I think the implementation relied on strncpy() setting a zero pad
> at the end of the string, so the ckeck would only be needed for a count
> value that starts out negative? Is there another way this can actually
> cause problems?

In kernel there is a common calling pattern is strncpy_from_user(buf,
user_ptr, sizeof(buf)), as I mentioned before. If the size of
user_ptr is greater than the buffer in the kernel, no zero attaches
to the end of copied string (see the implementation in lib/string.c).
So the checking of the count variable in this boolean condition does
not protect the tmp buffer in the last iteration of this loop in the
__strncpy_from_user.

Yangxi Xiang

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Arnd Bergmann]

On Mon, Jun 27, 2022 at 11:38 AM Yangxi Xiang <xyangxi5@gmail.com> wrote:
>
> > Which architectures do you mean? I don't see any architecture using
> > asm-generic/uaccess.h without setting GENERIC_STRNCPY_FROM_USER
> > before commit 98b861a30431 or the prior release.
>
> I am a user of LibOS, which uses this __strncpy_from_user.

Ok, got it. This should be part of the changelog then when you send a
patch for stable  kernels. You should also indicate that the code was
removed in mainline kernels and what the fix was there, as well as
which of the older kernels need the fix.

> > Also, I think the implementation relied on strncpy() setting a zero pad
> > at the end of the string, so the ckeck would only be needed for a count
> > value that starts out negative? Is there another way this can actually
> > cause problems?
>
> In kernel there is a common calling pattern is strncpy_from_user(buf,
> user_ptr, sizeof(buf)), as I mentioned before. If the size of
> user_ptr is greater than the buffer in the kernel, no zero attaches
> to the end of copied string (see the implementation in lib/string.c).
> So the checking of the count variable in this boolean condition does
> not protect the tmp buffer in the last iteration of this loop in the
> __strncpy_from_user.

Ah right, of course.

       Arnd

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Yangxi Xiang]

> You should also indicate that the code was
> removed in mainline kernels and what the fix was there, as well as
> which of the older kernels need the fix.

I have no idea on how to decide the range. The suggested version 
range might be >= 5.1. And, should I send the patch again according to
our discussion?

Thank you!

Yangxi Xiang

Re: [PATCH] asm-generic: fix buffer overflow read in __strncpy_from_user

[Greg KH]

On Mon, Jun 27, 2022 at 12:19:18PM +0200, Arnd Bergmann wrote:
> On Mon, Jun 27, 2022 at 11:38 AM Yangxi Xiang <xyangxi5@gmail.com> wrote:
> >
> > > Which architectures do you mean? I don't see any architecture using
> > > asm-generic/uaccess.h without setting GENERIC_STRNCPY_FROM_USER
> > > before commit 98b861a30431 or the prior release.
> >
> > I am a user of LibOS, which uses this __strncpy_from_user.
> 
> Ok, got it. This should be part of the changelog then when you send a
> patch for stable  kernels. You should also indicate that the code was
> removed in mainline kernels and what the fix was there, as well as
> which of the older kernels need the fix.

But libos isn't part of the kernel tree, that's an out-of-tree change
that we can't control here, right?  How is that relevant for stable
kernels and why can't it just be added to the libos patchset?

thanks,

greg k-h