* [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic @ 2021-09-18 15:05 Len Baker 2021-09-20 5:58 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Len Baker @ 2021-09-18 15:05 UTC (permalink / raw) To: Henrique de Moraes Holschuh, Hans de Goede, Mark Gross Cc: Len Baker, Gustavo A. R. Silva, Kees Cook, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel 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@gmx.com> --- 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)); 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); 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; return &sobj->s; -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-18 15:05 [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic Len Baker @ 2021-09-20 5:58 ` Kees Cook 2021-09-21 13:46 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2021-09-20 5:58 UTC (permalink / raw) To: Len Baker Cc: Henrique de Moraes Holschuh, Hans de Goede, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel 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@gmx.com> > --- > 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? (I see the caller uses +2? Why? It seems to be using each of hotkey_attributes, plus 1 more attr, plus a final NULL?) > 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? Why is struct attribute_set_obj marked as __packed? > > return &sobj->s; > -- > 2.25.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-20 5:58 ` Kees Cook @ 2021-09-21 13:46 ` Hans de Goede 2021-09-21 15:15 ` Greg KH 2021-09-25 10:37 ` Len Baker 0 siblings, 2 replies; 10+ messages in thread From: Hans de Goede @ 2021-09-21 13:46 UTC (permalink / raw) To: Kees Cook, Len Baker Cc: Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel 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@gmx.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-21 13:46 ` Hans de Goede @ 2021-09-21 15:15 ` Greg KH 2021-09-21 15:38 ` Hans de Goede 2021-09-25 10:40 ` Len Baker 2021-09-25 10:37 ` Len Baker 1 sibling, 2 replies; 10+ messages in thread From: Greg KH @ 2021-09-21 15:15 UTC (permalink / raw) To: Hans de Goede Cc: Kees Cook, Len Baker, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel 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@gmx.com> > >> --- > >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-21 15:15 ` Greg KH @ 2021-09-21 15:38 ` Hans de Goede 2021-09-21 15:45 ` Greg KH 2021-09-25 10:40 ` Len Baker 1 sibling, 1 reply; 10+ messages in thread From: Hans de Goede @ 2021-09-21 15:38 UTC (permalink / raw) To: Greg KH Cc: Kees Cook, Len Baker, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel 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@gmx.com> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-21 15:38 ` Hans de Goede @ 2021-09-21 15:45 ` Greg KH 0 siblings, 0 replies; 10+ messages in thread From: Greg KH @ 2021-09-21 15:45 UTC (permalink / raw) To: Hans de Goede Cc: Kees Cook, Len Baker, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel On Tue, Sep 21, 2021 at 05:38:39PM +0200, Hans de Goede wrote: > > 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 They should also go into Documentation/ABI/ which is where sysfs files are documented. We are working on tools that make parsing that easier, so it would be good to keep them out of other random documentation files whenever possible. thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-21 15:15 ` Greg KH 2021-09-21 15:38 ` Hans de Goede @ 2021-09-25 10:40 ` Len Baker 2021-09-25 11:07 ` Greg KH 1 sibling, 1 reply; 10+ messages in thread From: Len Baker @ 2021-09-25 10:40 UTC (permalink / raw) To: Greg KH Cc: Hans de Goede, Kees Cook, Len Baker, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel Hi, On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > 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? Is [1] the correct way to deal with devices attributes? I think so. [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes > > 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. Sorry but what is the default way? Would it be correct to check if the file exists? > > 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? What is the default way? Would it be correct to use device_create_file() and device_remove_file()? Sorry if it is a trivial question but I am a kernel newbie :) I have a lot to learn. Any suggestion or a good driver to look at would be greatly appreciated. Thanks, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-25 10:40 ` Len Baker @ 2021-09-25 11:07 ` Greg KH 2021-09-25 13:33 ` Len Baker 0 siblings, 1 reply; 10+ messages in thread From: Greg KH @ 2021-09-25 11:07 UTC (permalink / raw) To: Len Baker Cc: Hans de Goede, Kees Cook, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel On Sat, Sep 25, 2021 at 12:40:44PM +0200, Len Baker wrote: > Hi, > > On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > > > 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? > > Is [1] the correct way to deal with devices attributes? I think so. > > [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes No, do not use driver_create_file(), see: http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ as a more up to date thing. Someone should fix that in-kernel documentation one day :) > > 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. > > Sorry but what is the default way? Would it be correct to check if the > file exists? Use the is_visable() callback for the attribute group to enable/disable the creation of the sysfs file. > > 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? > > What is the default way? Would it be correct to use device_create_file() > and device_remove_file()? Put all the attributes into an attribute group and attach it to the driver. The driver core will create/remove the files when needed. The link above should help explain that a bit better, and I can point you at examples if needed. Does that help? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-25 11:07 ` Greg KH @ 2021-09-25 13:33 ` Len Baker 0 siblings, 0 replies; 10+ messages in thread From: Len Baker @ 2021-09-25 13:33 UTC (permalink / raw) To: Greg KH Cc: Len Baker, Hans de Goede, Kees Cook, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel Hi Greg, On Sat, Sep 25, 2021 at 01:07:20PM +0200, Greg KH wrote: > On Sat, Sep 25, 2021 at 12:40:44PM +0200, Len Baker wrote: > > Hi, > > > > On Tue, Sep 21, 2021 at 05:15:35PM +0200, Greg KH wrote: > > > > > > 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? > > > > Is [1] the correct way to deal with devices attributes? I think so. > > > > [1] https://www.kernel.org/doc/html/latest/driver-api/driver-model/driver.html#attributes > > No, do not use driver_create_file(), see: > http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ > as a more up to date thing. Ok, understood. Thanks. > > Someone should fix that in-kernel documentation one day :) > > > > 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. > > > > Sorry but what is the default way? Would it be correct to check if the > > file exists? > > Use the is_visable() callback for the attribute group to enable/disable > the creation of the sysfs file. Ok, I will take a look at it. > > > > 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? > > > > What is the default way? Would it be correct to use device_create_file() > > and device_remove_file()? > > Put all the attributes into an attribute group and attach it to the > driver. The driver core will create/remove the files when needed. The > link above should help explain that a bit better, and I can point you at > examples if needed. > > Does that help? Yes, things are clearer to me now. Also, since the only way to learn is to do so, I will take the task to switch this driver to the common use of attributes. Thank you very much for your time and guidance. Regards, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic 2021-09-21 13:46 ` Hans de Goede 2021-09-21 15:15 ` Greg KH @ 2021-09-25 10:37 ` Len Baker 1 sibling, 0 replies; 10+ messages in thread From: Len Baker @ 2021-09-25 10:37 UTC (permalink / raw) To: Hans de Goede, Kees Cook Cc: Len Baker, Henrique de Moraes Holschuh, Mark Gross, Gustavo A. R. Silva, ibm-acpi-devel, platform-driver-x86, linux-hardening, linux-kernel Hi, On Tue, Sep 21, 2021 at 03:46:23PM +0200, Hans de Goede wrote: > On 9/20/21 7:58 AM, Kees Cook wrote: > > On Sat, Sep 18, 2021 at 05:05:00PM +0200, Len Baker wrote: > >> > >> 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. Hans, thanks for the explanation. Yes, this is the reason I added the "plus 1". Not only based on the comment :) Regards, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-25 13:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-18 15:05 [PATCH] platform/x86: thinkpad_acpi: Prefer struct_size over open coded arithmetic Len Baker 2021-09-20 5:58 ` Kees Cook 2021-09-21 13:46 ` Hans de Goede 2021-09-21 15:15 ` Greg KH 2021-09-21 15:38 ` Hans de Goede 2021-09-21 15:45 ` Greg KH 2021-09-25 10:40 ` Len Baker 2021-09-25 11:07 ` Greg KH 2021-09-25 13:33 ` Len Baker 2021-09-25 10:37 ` Len Baker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).