uaccess: Add minimum bounds check on kernel buffer size
PATCH 1/1 uaccess: Add minimum bounds check on kernel buffer size
While there is logic about the difference between ksize and usize, copy_struct_from_user() didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possible memset() call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13:
In function 'copy_struct_from_user',
inlined from 'iommufd_fops_ioctl' at../drivers/iommu/iommufd/main.c:333:8:../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'union ucmd_buffer' [-Warray-bounds=]
59 | #define __underlying_memset __builtin_memset
| ^
../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset'
453 | __underlying_memset(p, c, __fortify_size); \
| ^~~~~~~~~~~~~~~~~~~
../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk'
461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \
| ^~~~~~~~~~~~~~~~~~~~
../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset'
334 | memset(dst + size, 0, rest);
| ^~~~~~../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl':../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here
311 | union ucmd_buffer buf;
| ^~~
--- include/linux/uaccess.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/uaccess.h +4 -0
@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,size_t size =min (ksize, usize);size_t rest =max (ksize, usize) - size; +/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +/* Deal with trailing bytes. */ if (usize < ksize) {memset (dst + size,0 , rest);
Signature · 1 line
2.34.1
Quoted 26 lines · Kees Cook
While there is logic about the difference between ksize and usize,
copy_struct_from_user()didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possiblememset()call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13:In function '
copy_struct_from_user',
inlined from 'iommufd_fops_ioctl' at../drivers/iommu/iommufd/main.c:333:8:../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'unionucmd_buffer' [-Warray-bounds=]59 | #define __underlying_memset __builtin_memset | ^ ../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset' 453 | __underlying_memset(p, c, __fortify_size); \ | ^~~~~~~~~~~~~~~~~~~ ../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk' 461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset' 334 | memset(dst + size, 0, rest); | ^~~~~~
Quoted 5 lines
../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl':../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here
311 | unionucmd_bufferbuf;
| ^~~
Hi Kees,
I started building with gcc-13.0.1 myself but ran into a lot of other -Warray-bounds warnings in randconfig builds, so I ended up turning it off once more with CONFIG_CC_NO_ARRAY_BOUNDS in order to keep building without warnings.
Is there anything else I need to do to get to the point of just addressing actual issues instead of false positives? Do you already have a patch series for fixing the others?
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, · line 336
Quoted 13 lines
include/linux/uaccess.h +4
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size; +/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE() may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
Arnd
Quoted 35 lines · Arnd Bergmann
Quoted 26 lines · Kees Cook
While there is logic about the difference between ksize and usize,
copy_struct_from_user()didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possiblememset()call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13:In function '
copy_struct_from_user',
inlined from 'iommufd_fops_ioctl' at../drivers/iommu/iommufd/main.c:333:8:../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'unionucmd_buffer' [-Warray-bounds=]59 | #define __underlying_memset __builtin_memset | ^ ../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset' 453 | __underlying_memset(p, c, __fortify_size); \ | ^~~~~~~~~~~~~~~~~~~ ../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk' 461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset' 334 | memset(dst + size, 0, rest); | ^~~~~~Quoted 5 lines
../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl':../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here
311 | unionucmd_bufferbuf;
| ^~~Hi Kees,
Quoted 8 lines
I started building with gcc-13.0.1 myself but ran into a lot of other -Warray-bounds warnings in randconfig builds, so I ended up turning it off once more with
CONFIG_CC_NO_ARRAY_BOUNDSin order to keep building without warnings.Is there anything else I need to do to get to the point of just addressing actual issues instead of false positives? Do you already have a patch series for fixing the others?
For reference, I just tried it out again and within half an hour I found about 30 unique warnings, which is way less than I had remembered, though some of these just show up a lot:
arch/arm/include/asm/io.h:113:9: error: array subscript 0 is outside array bounds of 'void[0]' [-Werror=array-bounds=]arch/arm/include/asm/io.h:95:9: error: array subscript 0 is outside array bounds of 'void[0]' [-Werror=array-bounds=] arch/arm/include/asm/memory.h:311:22: error: array subscript -1 is outside array bounds of 'char[2147483647]' [-Werror=array-bounds=] arch/arm64/kernel/setup.c:228:56: error: array subscript -1 is outside array bounds of 'char[]' [-Werror=array-bounds=]arch/x86/include/asm/io.h:56:3: error: array subscript 0 is outside array bounds of 'void[0]' [-Werror=array-bounds=] drivers/cpufreq/brcmstb-avs-cpufreq.c:449:28: error: array subscript 5 is outside array bounds of 'void[60]' [-Werror=array-bounds=] drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:329:46: error: array subscript 0 is outside array bounds of 'struct virt_dma_desc[24403223]' [-Werror=array-bounds=] drivers/gpu/drm/nouveau/nvif/outp.c:140:9: error: array subscript 'unsigned char[16][0]' is partly outside array bounds of 'u8[15]' {aka 'unsigned char[15]'} [-Werror=array-bounds=] drivers/hwmon/lm85.c:1110:26: error: array subscript [0, 31] is outside array bounds of 'struct lm85_autofan[3]' [-Werror=array-bounds=] drivers/infiniband/core/user_mad.c:564:50: error: array subscript 'struct ib_rmpp_mad[0]' is partly outside array bounds of 'unsigned char[140]' [-Werror=array-bounds=] drivers/infiniband/core/user_mad.c:566:42: error: array subscript 'struct ib_rmpp_mad[0]' is partly outside array bounds of 'unsigned char[140]' [-Werror=array-bounds=] drivers/infiniband/core/user_mad.c:618:25: error: array subscript 'struct ib_rmpp_mad[0]' is partly outside array bounds of 'unsigned char[140]' [-Werror=array-bounds=] drivers/infiniband/core/user_mad.c:622:44: error: array subscript 'struct ib_rmpp_mad[0]' is partly outside array bounds of 'unsigned char[140]' [-Werror=array-bounds=] drivers/media/tuners/mxl5005s.c:3429:72: error: array subscript 32 is above array bounds of 'u16[25]' {aka 'short unsigned int[25]'} [-Werror=array-bounds=] drivers/media/tuners/mxl5005s.c:3448:70: error: array subscript 32 is above array bounds of 'u16[25]' {aka 'short unsigned int[25]'} [-Werror=array-bounds=] drivers/spi/spi-imx.c:1687:34: error: array subscript 0 is outside array bounds of 'const bool[0]' {aka 'const _Bool[]'} [-Werror=array-bounds=] drivers/staging/rtl8712/rtl871x_xmit.c:949:40: error: array subscript 4 is outside array bounds of 'void[448]' [-Werror=array-bounds=] drivers/staging/rtl8712/rtl871x_xmit.c:950:39: error: array subscript 4 is outside array bounds of 'void[448]' [-Werror=array-bounds=] drivers/thermal/hisi_thermal.c:430:21: error: array subscript 1 is outside array bounds of 'void[20]' [-Werror=array-bounds=] drivers/thermal/hisi_thermal.c:431:21: error: array subscript 1 is outside array bounds of 'void[20]' [-Werror=array-bounds=] drivers/thermal/hisi_thermal.c:432:21: error: array subscript 1 is outside array bounds of 'void[20]' [-Werror=array-bounds=] drivers/usb/host/xhci-mvebu.c:37:28: error: array subscript [0, 89478485] is outside array bounds of 'const struct mbus_dram_window[0]' [-Werror=array-bounds=] drivers/usb/storage/ene_ub6250.c:1050:44: error: array subscript 'struct ms_bootblock_idi[0]' is partly outside array bounds of 'unsigned char[512]' [-Werror=array-bounds=] fs/btrfs/sysfs.c:636:13: error: array subscript -18 is outside array bounds of 'struct kobject[59652323]' [-Werror=array-bounds=] fs/crypto/fname.c:506:33: error: array subscript 'struct fscrypt_nokey_name[0]' is partly outside array bounds of 'unsigned char[189]' [-Werror=array-bounds=] include/linux/fortify-string.h:25:34: error: array subscript 0 is outside array bounds of 'const char[0]' [-Werror=array-bounds=] include/linux/fortify-string.h:57:33: error: array subscript 'unsigned char[16][0]' is partly outside array bounds of 'u8[15]' {aka 'unsigned char[15]'} [-Werror=array-bounds=] net/socket.c:651:21: error: array subscript -1 is outside array bounds of 'struct inode[3016128]' [-Werror=array-bounds=]net/socket.c:652:26: error: array subscript -1 is outside array bounds of 'struct inode[3016128]' [-Werror=array-bounds=]
If you like, I can run this for a while longer to get to something close to a complete list.
Arnd
Quoted 33 lines · Arnd Bergmann
Quoted 26 lines · Kees Cook
While there is logic about the difference between ksize and usize,
copy_struct_from_user()didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possiblememset()call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13:In function '
copy_struct_from_user',
inlined from 'iommufd_fops_ioctl' at../drivers/iommu/iommufd/main.c:333:8:../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'unionucmd_buffer' [-Warray-bounds=]59 | #define __underlying_memset __builtin_memset | ^ ../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset' 453 | __underlying_memset(p, c, __fortify_size); \ | ^~~~~~~~~~~~~~~~~~~ ../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk' 461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset' 334 | memset(dst + size, 0, rest); | ^~~~~~Quoted 5 lines
../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl':../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here
311 | unionucmd_bufferbuf;
| ^~~
Quoted 6 lines
Hi Kees,
I started building with gcc-13.0.1 myself but ran into a lot of other -Warray-bounds warnings in randconfig builds, so I ended up turning it off once more with
CONFIG_CC_NO_ARRAY_BOUNDSin order to keep building without warnings.
Understood. AFAIK, all the open bugs I (and you) filed with GCC 13 have been fixed related to -Warray-bounds. The most recent was the misbehavior between CONFIG_UBSAN_SHIFT and -Warray-bounds. (Though the shift checking still exposes some warnings since it introduces an implicit bounds check on the shift variable, but they're not _wrong_ any more.)
Quoted 3 lines
Is there anything else I need to do to get to the point of just addressing actual issues instead of false positives? Do you already have a patch series for fixing the others?
I've been working through the list that I see when building with -Warray-bounds and -fstrict-flex-arrays=3. Some are real bugs, as usual.
Quoted 9 lines
Quoted 9 lines
include/linux/uaccess.h +0
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size;
Quoted 8 lines
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, const void __user *src, · line 335
Quoted 4 lines
include/linux/uaccess.h +4
-0+/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE()may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
Yeah. IMO, copy_struct_from_user() is not fast path and having better bounds checking when coming from userspace is well worth it.
Signature · 1 line
Kees Cook
Hi Arnd,
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Kees Cook
include/linux/uaccess.h +0
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size;
Quoted 8 lines
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, const void __user *src, · line 335
Quoted 4 lines
include/linux/uaccess.h +4
-0+/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE()may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
When seeing this, I was a bit worried about the size increase. Hence I gave it a try on atari_defconfig and ran bloat-o-meter. Surprisingly, there was no size increase at all, as all checks were optimized away.
Hence perhaps this can become a compile-time check?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Quoted 20 lines · Geert Uytterhoeven
Hi Arnd,
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Kees Cook
include/linux/uaccess.h +0
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size;Quoted 8 lines
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, const void __user *src, · line 335
Quoted 4 lines
include/linux/uaccess.h +4
-0+/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE()may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
Quoted 6 lines
When seeing this, I was a bit worried about the size increase. Hence I gave it a try on
atari_defconfigand ran bloat-o-meter. Surprisingly, there was no size increase at all, as all checks were optimized away.Hence perhaps this can become a compile-time check?
Normally it should optimize away, yes.
Signature · 1 line
Kees Cook
A message that is not in the archive (sent off-list or not yet received): CAMuHMdXXSwYYoUMskhcgjF9SVjraZC-UsBT3sN+xkcUAYmJj4Q-2143@mail.gmail.com
Hi,
Le 06/02/2023 à 21:03, Geert Uytterhoeven a écrit :
Quoted 18 lines
Hi Arnd,
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Kees Cook
include/linux/uaccess.h +0
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size;Quoted 7 lines
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, const void __user *src, · line 335
Quoted 4 lines
include/linux/uaccess.h +4
-0+/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE()may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
Quoted 6 lines
When seeing this, I was a bit worried about the size increase. Hence I gave it a try on
atari_defconfigand ran bloat-o-meter. Surprisingly, there was no size increase at all, as all checks were optimized away.Hence perhaps this can become a compile-time check?
It should be a compile-time check, because one would not want __builtin_object_size(dst, 1) to return -1 if dst' size is not known at compile-time.
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Quoted 22 lines · Yann Droneaud
Hi,
Le 06/02/2023 à 21:03, Geert Uytterhoeven a écrit :
Quoted 18 lines
Hi Arnd,
Quoted 15 lines · Arnd Bergmann
include/linux/uaccess.h copy_struct_from_user(void *dst, size_t ksize, · line 336
Quoted 11 lines · Kees Cook
include/linux/uaccess.h +4
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize,const void __user *src, size_t size = min(ksize, usize);size_t rest =max (ksize, usize) - size; +/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +
WARN_ON_ONCE()may be a little expensive since that adds two comparisons and a static variable to each copy, but it's probably fine.
Quoted 8 lines
Quoted 6 lines
When seeing this, I was a bit worried about the size increase. Hence I gave it a try on
atari_defconfigand ran bloat-o-meter. Surprisingly, there was no size increase at all, as all checks were optimized away.Hence perhaps this can become a compile-time check?
It should be a compile-time check, because one would not want
__builtin_object_size(dst, 1)to return -1 if dst' size is not known at compile-time.
Note that it's size_t, so it's actually SIZE_MAX, which is why these tests will vanish most of the time. i.e. it cannot ever be possible for the SIZE_MAX case so the entire test is elided.
And when ksize is known at compile time and __bos is not SIZE_MAX, the result is also known to be either always true or always false, etc.
What's nice here is that when ksize is only run-time known and the buffer size is compile-time known, we'll keep the test.
Signature · 1 line
Kees Cook
Quoted 22 lines · Kees Cook
While there is logic about the difference between ksize and usize,
copy_struct_from_user()didn't check the size of the destination buffer (when it was known) against ksize. Add this check so there is an upper bounds check on the possiblememset()call, otherwise lower bounds checks made by callers will trigger bounds warnings under -Warray-bounds. Seen under GCC 13:In function '
copy_struct_from_user',
inlined from 'iommufd_fops_ioctl' at../drivers/iommu/iommufd/main.c:333:8:../include/linux/fortify-string.h:59:33: warning: '__builtin_memset' offset [57, 4294967294] is out of the bounds [0, 56] of object 'buf' with type 'unionucmd_buffer' [-Warray-bounds=]59 | #define __underlying_memset __builtin_memset | ^ ../include/linux/fortify-string.h:453:9: note: in expansion of macro '__underlying_memset' 453 | __underlying_memset(p, c, __fortify_size); \ | ^~~~~~~~~~~~~~~~~~~ ../include/linux/fortify-string.h:461:25: note: in expansion of macro '__fortify_memset_chk' 461 | #define memset(p, c, s) __fortify_memset_chk(p, c, s, \ | ^~~~~~~~~~~~~~~~~~~~ ../include/linux/uaccess.h:334:17: note: in expansion of macro 'memset' 334 | memset(dst + size, 0, rest); | ^~~~~~
Quoted 4 lines
../drivers/iommu/iommufd/main.c: In function 'iommufd_fops_ioctl':../drivers/iommu/iommufd/main.c:311:27: note: 'buf' declared here
311 | unionucmd_bufferbuf;
| ^~~
Makes sense.
Quoted 33 lines
Cc Aleksa Sarai <[email protected]>Cc Christian Brauner <[email protected]>Cc Rasmus Villemoes <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Catalin Marinas <[email protected]>Cc Andrew Morton <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Alexander Potapenko <[email protected]>Signed-off-by Kees Cook <[email protected]>--- include/linux/uaccess.h | 4 ++++ 1 file changed, 4 insertions(+)include/linux/uaccess.h +4
-0@@ -329,6 +329,10 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,size_t size =min (ksize, usize);size_t rest =max (ksize, usize) - size; +/* Double check if ksize is larger than a known object size. */ +if (WARN_ON_ONCE (ksize >__builtin_object_size (dst,1 ))) +return -E2BIG ; +/* Deal with trailing bytes. */ if (usize < ksize) {memset (dst + size,0 , rest);Signature · 1 line
2.34.1
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>