Bug 217885
| Summary: | Potential byte buffer overflows at snprintf() usages | ||
|---|---|---|---|
| Product: | Linux | Reporter: | Erich Löw (Erich.Loew) |
| Component: | Kernel | Assignee: | drivers_video-other |
| Status: | NEW --- | ||
| Severity: | normal | CC: | heiko.carstens, nathan, sam |
| Priority: | P3 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 6.5.3 | Subsystem: | drivers |
| Regression: | No | Bisected commit-id: | |
I found in drivers/scsi/myrs.c
Function: rebuild_show()
Code: if (sdev->channel < cs->ctlr_info->physchan_present)
return snprintf(buf, 32, "physical device - not rebuilding\n");
I found in drivers/scsi/myrb.c
Function: rebuild_show()
Code: if (sdev->channel < myrb_logical_channel(sdev->host))
return snprintf(buf, 32, "physical device - not rebuilding\n");
If in both cases the "buf" hosts up to 32 bytes --> potential buffer
overflow because the passed strings consist of 34 bytes.
snprintf() takes over only 32 bytes and hence no overflow, but the passed
strings are truncated to something not 0 terminated and this creates un-terminated strings...
Hence over whole kernel all usages of snprintf() need review to ensure that
all created strings are 0-terminated.
Please CC your findings to <security at kernel.org> I do so. Best THX for hint! The -Wall covers at clang-18 the warning -Wfortify-source
--> This warning checks at each occurrence of snprintf()
if the passed size (arg2) is less or equal than the size of the expansion
of optional args 4 .... n as per arg3 (the formatting sting)
In case of size of expanded string overflows the passed arg2: the
compiler warns and stops compilation.
--> Potential mitigations:
A) Reviewing all snprintf() occurrences (7969 pieces at least)
over whole kernel source.
B) Adding to master Makefile += "-Wno-fortify-source"
In my view: A) is the right way
B) is just a fixing hack for time being.
I checked older kernels... it is in since ever. clang-18 makes this observation now visible snprintf() truncates as per arg2: this prevents buffer overflows... in addition a pipe cleaning of all snprintf() calls might be considered? Because. In addition the truncations might produce not 0 terminated strings in area pointed to by arg1 of snprintf() The snprintf() man page (https://linux.die.net/man/3/snprintf) itself warns on the not null terminating risks... Bugs Because sprintf() and vsprintf() assume an arbitrarily long string, callers must be careful not to overflow the actual space; this is often impossible to assure. Note that the length of the strings produced is locale-dependent and difficult to predict. Use snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf(3)). Linux libc4.[45] does not have a snprintf(), but provides a libbsd that contains an snprintf() equivalent to sprintf(), that is, one that ignores the size argument. Thus, the use of snprintf() with early libc4 leads to serious security problems. Code such as printf(foo); often indicates a bug, since foo may contain a % character. If foo comes from untrusted user input, it may contain %n, causing the printf() call to write to memory and creating a security hole. I found in kernel 6.5.3 changelog the following commit 614cc44384bf4692e2d545ee9c830fcfc991e11f Author: Heiko Carstens <hca@linux.ibm.com> Date: Mon Aug 28 17:31:42 2023 +0200 s390/dasd: fix string length handling commit f7cf22424665043787a96a66a048ff6b2cfd473c upstream. Building dasd_eckd.o with latest clang reveals this bug: CC drivers/s390/block/dasd_eckd.o drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 11 [-Wfortify-source] 1082 | snprintf(print_uid, sizeof(*print_uid), | ^ drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated; specified size is 1, but format string expands to at least 10 [-Wfortify-source] 1087 | snprintf(print_uid, sizeof(*print_uid), | ^ Fix this by moving and using the existing UID_STRLEN for the arrays that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN to clarify its scope. Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf") Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> Signed-off-by: Heiko Carstens <hca@linux.ibm.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> # build Reported-by: Nathan Chancellor <nathan@kernel.org> Closes: https://github.com/ClangBuiltLinux/linux/issues/1923 Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Link: https://lore.kernel.org/r/20230828153142.2843753-2-hca@linux.ibm.com Signed-off-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --------------------------------------------------------------------------- Hence developers partially attack the topic.... But I guess that more effort to be considered to pipe clean al 7769 occurrences --------------------------------------------------------------------------- Hi Erich, Thanks for all the work you have done here! There are two aspects to this issue. The first are the warnings themselves, which should indeed be fixed. The one you mention in the first comment has already been fixed in mainline in https://git.kernel.org/linus/a9415b03f0213fdf8194e128980445a0d692cd5d. The second is why you are seeing these warnings with clang but not with older clang or GCC. This warning is the same as GCC's -Wformat-truncation, which has been disabled in the kernel since https://git.kernel.org/linus/bd664f6b3e376a8ef4990f87d08271cc2d01ba9a. When it was added recently in clang (https://github.com/llvm/llvm-project/commit/0c9c9dd9a24f9d715d950fef0ac7aae01437af96), it was added to -Wfortify-source rather than added as -Wformat-truncation, so it did not get disabled for clang like it is for GCC. This is being fixed in clang (https://github.com/llvm/llvm-project/pull/65969), so once that lands, these warnings will be hidden due to -Wno-format-truncation in a normal build and on in W=1 builds (https://git.kernel.org/linus/6d4ab2e97dcfbcd748ae71761a9d8e5e41cc732c), so that they can be fixed over time. For the record, most folks don't really check Bugzilla. For issues that you see when building the kernel with clang/LLVM but not with GCC, you'll get a quicker response if you file an issue on our issue tracker (where there is already an issue about this): https://github.com/ClangBuiltLinux/linux/issues https://github.com/ClangBuiltLinux/linux/issues/1923 If you want to report the instances that pop up from the warning if they show up with GCC (as I note in the second comment of the above issue), you could start sending reports to the maintainers of those files using scripts/get_maintainer.pl. Cheers, Nathan |
Kernel: 6.5.2 Function: neo_alloc_fb_info() Observation: at case label FB_ACCEL_NEOMAGIC_NM2097 the helper function snprintf() attempts to copy to info->fix.id the string "MagicGraph 128ZV+". This requires 18 bytes of spaces. struct member "id" can only host 16 bytes as per declaration struct fb_fix_screeninfo { char id[16]; /* identification string eg "TT Builtin" */ unsigned long smem_start; /* Start of frame buffer mem */ /* (physical address) */ __u32 smem_len; /* Length of frame buffer mem */ __u32 type; /* see FB_TYPE_* */ __u32 type_aux; /* Interleave for interleaved Planes */ __u32 visual; /* see FB_VISUAL_* */ __u16 xpanstep; /* zero if no hardware panning */ __u16 ypanstep; /* zero if no hardware panning */ __u16 ywrapstep; /* zero if no hardware ywrap */ __u32 line_length; /* length of a line in bytes */ unsigned long mmio_start; /* Start of Memory Mapped I/O */ /* (physical address) */ __u32 mmio_len; /* Length of Memory Mapped I/O */ __u32 accel; /* Indicate to driver which */ /* specific chip/card we have */ __u16 capabilities; /* see FB_CAP_* */ __u16 reserved[2]; /* Reserved for future compatibility */ }; I found this due to compiling the kernel with "clang version 18.0.0 LATEST" --> This LATEST clang compiler reports this and further overflows in function neo_alloc_fb_info(). --> gcc (GCC) 14.0.0 and former clang incarnations do not detect this observation. A potential mitigation: declaring the member id as id[24] --> But this is maybe introducing other trouble --> And if in future some fb device uses an identifier string exceeding 24 bytes, the trouble regresses.