linux-kernel - 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: <202503041935.AE2093CFFA@keescook>
Date: Tue, 4 Mar 2025 20:57:16 -0800
From: Kees Cook <kees@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>,
	Qing Zhao <qing.zhao@...cle.com>, Bill Wendling <morbo@...gle.com>
Cc: Peter Rosin <peda@...ntia.se>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-kernel@...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 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?

-- 
Kees Cook