Re: [RESEND PATCH] mux: Convert mux_control_ops to a flex array member in mux_chip

[Kees Cook <kees@kernel.org>]

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