linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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

* 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

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).