Hi, On 9/21/21 5:15 PM, Greg KH wrote: > On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote: >> 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 :) > > First off, why is a single driver doing so many odd things with > attribute groups? Why not just use them the way that the rest of the > kernel does? Why does this driver need this special handling and no one > else does? The thinkpad_acpi driver carries a lot of legacy with it. So in general we are careful with making changes because some people still use quite old ThinkPad-s and it is tricky to make sure we don't break stuff on older models. So yeah there is some cruft in a bunch of places in this driver. In this case it seems that cleaning things up is a straight forward fix though, so we really should do so. > > I think the default way of handling if an attribute is enabled or not, > should suffice here, and make things much simpler overall as all of this > crazy attribute handling can just be removed. > > Bonus would also be that I think it would fix the race conditions that > happen when trying to create attributes after the device is bound to the > driver that I think the existing driver has today. > >>> (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. > > Again, using the default way to show (or not show) attributes should > solve this issue. Why not just use that instead? I agree, moving to a "fixed" attribute_group, with an is_visible check for the optional attributes would be a much better fix and allow removal of a whole bunch of custom code. If anyone following this thread could submit a patch doing that, then that would be great. >>>> 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. > > It is used by some, that is how you can put all of the attributes in a > subdirectory automatically. No idea if that's needed here... > > All attributes for this driver are documented in Documentation/ABI/, > right? :) I'm not sure if all attributes are documented, but a lot of them (including all recently added ones) are documented in: Documentation/admin-guide/laptops/thinkpad-acpi.rst Regards, Hans
