Hi, On 9/20/21 7:58 AM, Kees Cook wrote: > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: >> As noted in the "Deprecated Interfaces, Language Features, Attributes, >> and Conventions" documentation [1], size calculations (especially >> multiplication) should not be performed in memory allocator (or similar) >> function arguments due to the risk of them overflowing. This could lead >> to values wrapping around and a smaller allocation being made than the >> caller was expecting. Using those allocations could lead to linear >> overflows of heap memory and other misbehaviors. >> >> So, switch to flexible array member in the struct attribute_set_obj and >> refactor the code accordingly to use the struct_size() helper instead of >> the argument "size + count * size" in the kzalloc() function. >> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments >> >> Signed-off-by: Len Baker <len.baker@xxxxxxx> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 50ff04c84650..ed0b01ead796 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -1008,7 +1008,7 @@ struct attribute_set { >> >> struct attribute_set_obj { >> struct attribute_set s; >> - struct attribute *a; >> + struct attribute *a[]; >> } __attribute__((packed)); > > Whoa. I have so many questions... :) > >> >> static struct attribute_set *create_attr_set(unsigned int max_members, >> @@ -1020,13 +1020,11 @@ static struct attribute_set *create_attr_set(unsigned int max_members, >> return NULL; >> >> /* Allocates space for implicit NULL at the end too */ >> - sobj = kzalloc(sizeof(struct attribute_set_obj) + >> - max_members * sizeof(struct attribute *), >> - GFP_KERNEL); >> + sobj = kzalloc(struct_size(sobj, a, max_members + 1), GFP_KERNEL); > > Whoa, this needs a lot more detail in the changelog if this is actually > correct. The original code doesn't seem to match the comment? (Where is > the +1?) So is this also a bug-fix? Kees, at first I thought you were spot-on with this comment, but the truth is more subtle. struct attribute_set_obj was: struct attribute_set_obj { struct attribute_set s; struct attribute *a; } __attribute__((packed)); Another way of looking at this, which makes things more clear is as: struct attribute_set_obj { struct attribute_set s; struct attribute *a[1]; } __attribute__((packed)); So the sizeof(struct attribute_set_obj) in the original kzalloc call included room for 1 "extra" pointer which is reserved for the terminating NULL pointer. Changing the struct to: struct attribute_set_obj { struct attribute_set s; struct attribute *a[]; } __attribute__((packed)); Is equivalent to changing it to: struct attribute_set_obj { struct attribute_set s; struct attribute *a[0]; } __attribute__((packed)); So the change in the struct declaration reduces the sizeof(struct attribute_set_obj) by the size of 1 pointer, making the +1 necessary. So AFAICT there is actually no functional change here. Still I will hold off merging this until we agree on this :) > (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, > plus 1 more attr, plus a final NULL?) The +2 is actually for 2 extra attributes (making the total number of extra attributes +3 because the sizeof(struct attribute_set_obj) already includes 1 extra). FWIW these 2 extra attributes are for devices with a a physical rfkill on/off switch and for the device being a convertible capable of reporting laptop- vs tablet-mode. >> if (!sobj) >> return NULL; >> sobj->s.max_members = max_members; >> - sobj->s.group.attrs = &sobj->a; >> + sobj->s.group.attrs = sobj->a; >> sobj->s.group.name = name; > > The caller also never sets a name? attribute_group.name may be NULL, I don't know of (m)any drivers which actual set this to non NULL. > Why is struct attribute_set_obj marked as __packed? I have no clue, this seems completely unnecessary. Len Baker can you submit a separate patch removing the useless __packed ? Regards, Hans
