Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic

[Greg KH]

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?

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?

> >>  	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? :)

thanks,

greg k-h