[v3] ACPI: platform-profile: Fix CFI violation when accessing sysfs files

When an attribute group is created with sysfs_create_group(), the
->sysfs_ops() callback is set to kobj_sysfs_ops, which sets the ->show()
and ->store() callbacks to kobj_attr_show() and kobj_attr_store()
respectively. These functions use container_of() to get the respective
callback from the passed attribute, meaning that these callbacks need to
be the same type as the callbacks in 'struct kobj_attribute'.

However, the platform_profile sysfs functions have the type of the
->show() and ->store() callbacks in 'struct device_attribute', which
results a CFI violation when accessing platform_profile or
platform_profile_choices under /sys/firmware/acpi because the types do
not match:

  CFI failure at kobj_attr_show+0x19/0x30 (target: platform_profile_choices_show+0x0/0x140; expected type: 0x7a69590c)

There is no functional issue from the type mismatch because the layout
of 'struct kobj_attribute' and 'struct device_attribute' are the same,
so the container_of() cast does not break anything aside from CFI.

Change the type of platform_profile_choices_show() and
platform_profile_{show,store}() to match the callbacks in
'struct kobj_attribute' and update the attribute variables to match,
which resolves the CFI violation.

Cc: stable@vger.kernel.org
Fixes: a2ff95e018f1 ("ACPI: platform: Add platform profile support")
Reported-by: John Rowley <lkml@johnrowley.me>
Closes: https://github.com/ClangBuiltLinux/linux/issues/2047
Tested-by: John Rowley <lkml@johnrowley.me>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Changes in v3:
- Rebase on 6.14-rc1, which includes updates to the driver to address
  Greg's previous concerns but this change is still needed for the
  legacy sysfs interface. v2 can be used for the stable backport.
- Link to v2: https://lore.kernel.org/r/20241118-acpi-platform_profile-fix-cfi-violation-v2-1-62ff952804de@kernel.org

Changes in v2:
- Rebase on linux-pm/acpi
- Pick up Sami's reviewed-by tag
- Adjust wording around why there is no functional issue from the
  mismatched types
- Link to v1: https://lore.kernel.org/r/20240819-acpi-platform_profile-fix-cfi-violation-v1-1-479365d848f6@kernel.org
---
 drivers/acpi/platform_profile.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)


---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20240819-acpi-platform_profile-fix-cfi-violation-de278753bd5f

Best regards,

On Mon, Feb 10, 2025 at 09:28:25PM -0500, Nathan Chancellor wrote:
> When an attribute group is created with sysfs_create_group(), the
> ->sysfs_ops() callback is set to kobj_sysfs_ops, which sets the ->show()
> and ->store() callbacks to kobj_attr_show() and kobj_attr_store()
> respectively. These functions use container_of() to get the respective
> callback from the passed attribute, meaning that these callbacks need to
> be the same type as the callbacks in 'struct kobj_attribute'.
> 
> However, the platform_profile sysfs functions have the type of the
> ->show() and ->store() callbacks in 'struct device_attribute', which
> results a CFI violation when accessing platform_profile or
> platform_profile_choices under /sys/firmware/acpi because the types do
> not match:
> 
>   CFI failure at kobj_attr_show+0x19/0x30 (target: platform_profile_choices_show+0x0/0x140; expected type: 0x7a69590c)
> 
> There is no functional issue from the type mismatch because the layout
> of 'struct kobj_attribute' and 'struct device_attribute' are the same,
> so the container_of() cast does not break anything aside from CFI.
> 
> Change the type of platform_profile_choices_show() and
> platform_profile_{show,store}() to match the callbacks in
> 'struct kobj_attribute' and update the attribute variables to match,
> which resolves the CFI violation.

Nice catch.

> Cc: stable@vger.kernel.org
> Fixes: a2ff95e018f1 ("ACPI: platform: Add platform profile support")
> Reported-by: John Rowley <lkml@johnrowley.me>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2047
> Tested-by: John Rowley <lkml@johnrowley.me>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> Changes in v3:
> - Rebase on 6.14-rc1, which includes updates to the driver to address
>   Greg's previous concerns but this change is still needed for the
>   legacy sysfs interface. v2 can be used for the stable backport.
> - Link to v2: https://lore.kernel.org/r/20241118-acpi-platform_profile-fix-cfi-violation-v2-1-62ff952804de@kernel.org

I'll never find that, so be prepared for a "FAILED" email when this hits
Linus's tree :)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Hi Nathan

On Mon, Feb 10, 2025, at 9:28 PM, Nathan Chancellor wrote:
> When an attribute group is created with sysfs_create_group(), the
> ->sysfs_ops() callback is set to kobj_sysfs_ops, which sets the ->show()
> and ->store() callbacks to kobj_attr_show() and kobj_attr_store()
> respectively. These functions use container_of() to get the respective
> callback from the passed attribute, meaning that these callbacks need to
> be the same type as the callbacks in 'struct kobj_attribute'.
>
> However, the platform_profile sysfs functions have the type of the
> ->show() and ->store() callbacks in 'struct device_attribute', which
> results a CFI violation when accessing platform_profile or
> platform_profile_choices under /sys/firmware/acpi because the types do
> not match:
>
>   CFI failure at kobj_attr_show+0x19/0x30 (target: 
> platform_profile_choices_show+0x0/0x140; expected type: 0x7a69590c)
>
> There is no functional issue from the type mismatch because the layout
> of 'struct kobj_attribute' and 'struct device_attribute' are the same,
> so the container_of() cast does not break anything aside from CFI.
>
> Change the type of platform_profile_choices_show() and
> platform_profile_{show,store}() to match the callbacks in
> 'struct kobj_attribute' and update the attribute variables to match,
> which resolves the CFI violation.
>
> Cc: stable@vger.kernel.org
> Fixes: a2ff95e018f1 ("ACPI: platform: Add platform profile support")
> Reported-by: John Rowley <lkml@johnrowley.me>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2047
> Tested-by: John Rowley <lkml@johnrowley.me>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> Changes in v3:
> - Rebase on 6.14-rc1, which includes updates to the driver to address
>   Greg's previous concerns but this change is still needed for the
>   legacy sysfs interface. v2 can be used for the stable backport.
> - Link to v2: 
> https://lore.kernel.org/r/20241118-acpi-platform_profile-fix-cfi-violation-v2-1-62ff952804de@kernel.org
>
> Changes in v2:
> - Rebase on linux-pm/acpi
> - Pick up Sami's reviewed-by tag
> - Adjust wording around why there is no functional issue from the
>   mismatched types
> - Link to v1: 
> https://lore.kernel.org/r/20240819-acpi-platform_profile-fix-cfi-violation-v1-1-479365d848f6@kernel.org
> ---
>  drivers/acpi/platform_profile.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c 
> b/drivers/acpi/platform_profile.c
> index fc92e43d0fe9..1b6317f759f9 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -260,14 +260,14 @@ static int _aggregate_choices(struct device *dev, 
> void *data)
> 
>  /**
>   * platform_profile_choices_show - Show the available profile choices 
> for legacy sysfs interface
> - * @dev: The device
> + * @kobj: The kobject
>   * @attr: The attribute
>   * @buf: The buffer to write to
>   *
>   * Return: The number of bytes written
>   */
> -static ssize_t platform_profile_choices_show(struct device *dev,
> -					     struct device_attribute *attr,
> +static ssize_t platform_profile_choices_show(struct kobject *kobj,
> +					     struct kobj_attribute *attr,
>  					     char *buf)
>  {
>  	unsigned long aggregate[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> @@ -333,14 +333,14 @@ static int _store_and_notify(struct device *dev, 
> void *data)
> 
>  /**
>   * platform_profile_show - Show the current profile for legacy sysfs interface
> - * @dev: The device
> + * @kobj: The kobject
>   * @attr: The attribute
>   * @buf: The buffer to write to
>   *
>   * Return: The number of bytes written
>   */
> -static ssize_t platform_profile_show(struct device *dev,
> -				     struct device_attribute *attr,
> +static ssize_t platform_profile_show(struct kobject *kobj,
> +				     struct kobj_attribute *attr,
>  				     char *buf)
>  {
>  	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> @@ -362,15 +362,15 @@ static ssize_t platform_profile_show(struct device *dev,
> 
>  /**
>   * platform_profile_store - Set the profile for legacy sysfs interface
> - * @dev: The device
> + * @kobj: The kobject
>   * @attr: The attribute
>   * @buf: The buffer to read from
>   * @count: The number of bytes to read
>   *
>   * Return: The number of bytes read
>   */
> -static ssize_t platform_profile_store(struct device *dev,
> -				      struct device_attribute *attr,
> +static ssize_t platform_profile_store(struct kobject *kobj,
> +				      struct kobj_attribute *attr,
>  				      const char *buf, size_t count)
>  {
>  	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
> @@ -401,12 +401,12 @@ static ssize_t platform_profile_store(struct device *dev,
>  	return count;
>  }
> 
> -static DEVICE_ATTR_RO(platform_profile_choices);
> -static DEVICE_ATTR_RW(platform_profile);
> +static struct kobj_attribute attr_platform_profile_choices = 
> __ATTR_RO(platform_profile_choices);
> +static struct kobj_attribute attr_platform_profile = 
> __ATTR_RW(platform_profile);
> 
>  static struct attribute *platform_profile_attrs[] = {
> -	&dev_attr_platform_profile_choices.attr,
> -	&dev_attr_platform_profile.attr,
> +	&attr_platform_profile_choices.attr,
> +	&attr_platform_profile.attr,
>  	NULL
>  };
> 
>
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20240819-acpi-platform_profile-fix-cfi-violation-de278753bd5f
>
> Best regards,
> -- 
> Nathan Chancellor <nathan@kernel.org>

Just a note to say thank you for noticing my email address change - the old lenovo one was awful to use so I dropped it a while back, but it meant I missed your first two patches (my mail filters for the list should have caught it and I'll have to figure out why not...)

Hi Mark,

On Tue, Feb 11, 2025 at 10:05:18AM -0500, Mark Pearson wrote:
> Just a note to say thank you for noticing my email address change -
> the old lenovo one was awful to use so I dropped it a while back, but
> it meant I missed your first two patches (my mail filters for the list
> should have caught it and I'll have to figure out why not...)

No worries, I totally get that. It may be good to update the few places
in the kernel that still use your Lenovo address but I understand if
there are other reasons to keep it around. .mailmap could also help too.

> Patch looks fine (I'd like to try it out on my system - will aim to do
> that today). I didn't know what CFI was (and have lightly educated
> myself now).  Is there a better way to fix this so it's common across
> all drivers somehow? Updating every individual instance is going to be
> a lot.

This does not happen all too often as far as I can tell. I have only
sent a handful of these patches over the past couple of years since CFI
was officially merged. Due to the way these sysfs interfaces work, I am
not sure this can really be fixed at a grand scale.

Cheers,
Nathan