Bug 217885 - Potential byte buffer overflows at snprintf() usages
Summary: Potential byte buffer overflows at snprintf() usages
Status: NEW
Alias: None
Product: Linux
Classification: Unclassified
Component: Kernel (show other bugs)
Hardware: All Linux
: P3 normal
Assignee: drivers_video-other
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-07 19:26 UTC by Erich Löw
Modified: 2023-11-29 07:31 UTC (History)
3 users (show)

See Also:
Kernel Version: 6.5.3
Subsystem: drivers
Regression: No
Bisected commit-id:


Attachments

Description Erich Löw 2023-09-07 19:26:34 UTC
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.
Comment 1 Erich Löw 2023-09-07 20:18:34 UTC
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.
Comment 2 Artem S. Tashkinov 2023-09-07 21:35:41 UTC
Please CC your findings to <security at kernel.org>
Comment 3 Erich Löw 2023-09-08 09:09:40 UTC
I do so. Best THX for hint!
Comment 4 Erich Löw 2023-09-08 09:26:47 UTC
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.
Comment 5 Erich Löw 2023-09-08 09:28:13 UTC
I checked older kernels... it is in since ever. clang-18 makes this observation
now visible
Comment 6 Erich Löw 2023-09-08 10:15:53 UTC
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()
Comment 7 Erich Löw 2023-09-11 13:17:48 UTC
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.
Comment 8 Erich Löw 2023-09-15 12:34:09 UTC
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
---------------------------------------------------------------------------
Comment 9 nathan 2023-09-15 17:00:13 UTC
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

Note You need to log in before you can comment on or make changes to this bug.