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); }
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
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Arnd Bergmann -
Greg KH -
Yangxi Xiang