On 09/22/23 at 10:52am, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct crash_mem.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Eric Biederman <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/linux/crash_core.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 3426f6eef60b..5126a4fecb44 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline,
> struct crash_mem {
> unsigned int max_nr_ranges;
> unsigned int nr_ranges;
> - struct range ranges[];
> + struct range ranges[] __counted_by(max_nr_ranges);
This __counted_by() only makes sense when there's a obvious upper
boundary, max_nr_ranges in this case. This heavily depends and isn't
much in kernel? E.g struct swap_info_struct->avail_lists[]. Just
curious, not related to this patch though.
> };
>
> extern int crash_exclude_mem_range(struct crash_mem *mem,
> --
> 2.34.1
>
On Sat, Sep 23, 2023 at 08:46:47AM +0800, Baoquan He wrote:
> On 09/22/23 at 10:52am, Kees Cook wrote:
> > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > attribute. Flexible array members annotated with __counted_by can have
> > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > functions).
> >
> > As found with Coccinelle[1], add __counted_by for struct crash_mem.
> >
> > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> >
> > Cc: Eric Biederman <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > include/linux/crash_core.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > index 3426f6eef60b..5126a4fecb44 100644
> > --- a/include/linux/crash_core.h
> > +++ b/include/linux/crash_core.h
> > @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline,
> > struct crash_mem {
> > unsigned int max_nr_ranges;
> > unsigned int nr_ranges;
> > - struct range ranges[];
> > + struct range ranges[] __counted_by(max_nr_ranges);
>
> This __counted_by() only makes sense when there's a obvious upper
> boundary, max_nr_ranges in this case.
Yes; it's designed to be the array element count used for the
allocation. For example with the above case:
nr_ranges += 2;
cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
if (!cmem)
return NULL;
cmem->max_nr_ranges = nr_ranges;
cmem->nr_ranges = 0;
nr_ranges is the max count of the elements.
_However_, if a structure (like this one) has _two_ counters, one for
"in use" and another for "max available", __counted_by could specify the
"in use" case, as long as array indexing only happens when that "in use"
has been updated. So, if it were:
struct crash_mem {
unsigned int max_nr_ranges;
unsigned int nr_ranges;
struct range ranges[] __counted_by(nr_ranges);
};
then this would trigger the bounds checking:
cmem->ranges[0] = some_range; /* "nr_ranges" is still 0 so index 0 isn't allowed */
cmem->nr_ranges ++;
but this would not:
cmem->nr_ranges ++; /* index 0 is now available for use. */
cmem->ranges[0] = some_range;
> This heavily depends and isn't much in kernel?
Which "this" do you mean? The tracking of max allocation is common.
Tracking max and "in use" happens in some places (like here), but is
less common.
> E.g struct swap_info_struct->avail_lists[].
This is even less common: tracking the count externally from the struct,
as done there with nr_node_ids. Shakeel asked a very similar question
and also pointed out nr_node_ids:
https://lore.kernel.org/all/202309221128.6AC35E3@keescook/
> Just curious, not related to this patch though.
I'm happy to answer questions! Yeah, as I said in the above thread,
I expect to expand what __counted_by can use, and I suspect (hope)
a global would be easier to add than an arbitrary expression. :)
-Kees
--
Kees Cook
On 09/22/23 at 08:25pm, Kees Cook wrote:
> On Sat, Sep 23, 2023 at 08:46:47AM +0800, Baoquan He wrote:
> > On 09/22/23 at 10:52am, Kees Cook wrote:
> > > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > > attribute. Flexible array members annotated with __counted_by can have
> > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > > functions).
> > >
> > > As found with Coccinelle[1], add __counted_by for struct crash_mem.
> > >
> > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> > >
> > > Cc: Eric Biederman <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Kees Cook <[email protected]>
> > > ---
> > > include/linux/crash_core.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > > index 3426f6eef60b..5126a4fecb44 100644
> > > --- a/include/linux/crash_core.h
> > > +++ b/include/linux/crash_core.h
> > > @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline,
> > > struct crash_mem {
> > > unsigned int max_nr_ranges;
> > > unsigned int nr_ranges;
> > > - struct range ranges[];
> > > + struct range ranges[] __counted_by(max_nr_ranges);
> >
> > This __counted_by() only makes sense when there's a obvious upper
> > boundary, max_nr_ranges in this case.
>
> Yes; it's designed to be the array element count used for the
> allocation. For example with the above case:
>
> nr_ranges += 2;
> cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> if (!cmem)
> return NULL;
>
> cmem->max_nr_ranges = nr_ranges;
> cmem->nr_ranges = 0;
>
> nr_ranges is the max count of the elements.
>
> _However_, if a structure (like this one) has _two_ counters, one for
> "in use" and another for "max available", __counted_by could specify the
> "in use" case, as long as array indexing only happens when that "in use"
> has been updated. So, if it were:
>
> struct crash_mem {
> unsigned int max_nr_ranges;
> unsigned int nr_ranges;
> struct range ranges[] __counted_by(nr_ranges);
> };
>
> then this would trigger the bounds checking:
>
> cmem->ranges[0] = some_range; /* "nr_ranges" is still 0 so index 0 isn't allowed */
> cmem->nr_ranges ++;
>
> but this would not:
>
> cmem->nr_ranges ++; /* index 0 is now available for use. */
> cmem->ranges[0] = some_range;
>
> > This heavily depends and isn't much in kernel?
>
> Which "this" do you mean? The tracking of max allocation is common.
> Tracking max and "in use" happens in some places (like here), but is
> less common.
I thought usually it may not have a max counter of the variable length
array embeded in struct, seems I was wrong. Here 'this' means the
__counted_by() adding for the variable length array.
>
> > E.g struct swap_info_struct->avail_lists[].
>
> This is even less common: tracking the count externally from the struct,
> as done there with nr_node_ids. Shakeel asked a very similar question
> and also pointed out nr_node_ids:
> https://lore.kernel.org/all/202309221128.6AC35E3@keescook/
>
> > Just curious, not related to this patch though.
>
> I'm happy to answer questions! Yeah, as I said in the above thread,
> I expect to expand what __counted_by can use, and I suspect (hope)
> a global would be easier to add than an arbitrary expression. :)
Thanks a lot for these explanation, Kees.
LGTM,
Acked-by: Baoquan He <[email protected]>