linux-hardening - Re: [RESEND PATCH] mux: Convert mux_control_ops to a flex array member in mux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20C5E493-4FB3-4347-844B-1AFE6B9FCDA4@oracle.com>
Date: Wed, 5 Mar 2025 17:31:57 +0000
From: Qing Zhao <qing.zhao@...cle.com>
To: Kees Cook <kees@...nel.org>
CC: Thorsten Blum <thorsten.blum@...ux.dev>, Bill Wendling <morbo@...gle.com>,
        Peter Rosin <peda@...ntia.se>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [RESEND PATCH] mux: Convert mux_control_ops to a flex array
 member in mux_chip



> On Mar 4, 2025, at 23:57, Kees Cook <kees@...nel.org> wrote:
> 
> On Tue, Mar 04, 2025 at 09:58:21AM +0100, Thorsten Blum wrote:
>> On 3. Mar 2025, at 19:44, Kees Cook wrote:
>>> On Mon, Mar 03, 2025 at 12:02:22AM +0100, Thorsten Blum wrote:
>>>> Convert mux_control_ops to a flexible array member at the end of the
>>>> mux_chip struct and add the __counted_by() compiler attribute to
>>>> improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>>>> CONFIG_FORTIFY_SOURCE.
>>>> 
>>>> Use struct_size() to calculate the number of bytes to allocate for a new
>>>> mux chip and to remove the following Coccinelle/coccicheck warning:
>>>> 
>>>> WARNING: Use struct_size
>>>> 
>>>> Use size_add() to safely add any extra bytes.
>>>> 
>>>> Compile-tested only.
>>> 
>>> I believe this will fail at runtime. Note that sizeof_priv follows the
>>> allocation, so at the very least, you'd need to update:
>>> 
>>> static inline void *mux_chip_priv(struct mux_chip *mux_chip)
>>> {
>>>      return &mux_chip->mux[mux_chip->controllers];
>>> }
>>> 
>>> to not use the mux array itself as a location reference because it will
>>> be seen as out of bounds.
>> 
>> Getting the address doesn't fail at runtime, does it? For this example
>> it works, but maybe I'm missing some compiler flag?
>> 
>> https://godbolt.org/z/qTEdqn9WW
> 
> Uhn. I can't explain that. :( We've seen this calculation get tripped
> in the real world, though:
> 
> https://git.kernel.org/linus/a26a5107bc52
> 
> But yeah, when I build local test cases, grabbing an integral trips it,
> but taking an address does not, as your godbolt shows. This makes no
> sense to me at all.
> 
> Here's my version, doing a direct comparison of int to *(int *) ...
> https://godbolt.org/z/e1bKGz739
> 
> #include <stdlib.h>
> #include <stdio.h>
> 
> struct foo {
>    int count;
>    int array[] __attribute__((__counted_by__(count)));
> };
> 
> int main(int argc, char *argv[]) {
>    int num_elems = 2 + argc;
> 
>    struct foo *p = malloc(sizeof(*p) + num_elems * sizeof(*p->array) + sizeof(int));
>    p->count = num_elems;
> 
>    // this correctly trips sanitizer:
>    int val = p->array[num_elems];
>    printf("%d\n", val);
> 
>    // this does not?!
>    int *valp = &p->array[num_elems];
>    printf("%p %d\n", valp, *valp);
> 
>    return 0;
> }
> 
> Qing and Bill, are you able to explain this? If I set p->count = 0, 1, or
> 2, this trips. Is this somehow an off-by-one error in both GCC and Clang?

This does look like a bug in the compiler, could you please file a bug against GCC on this?

thanks.

Qing
> 
> -- 
> Kees Cook