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

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Aug 19, 2024 at 12:09:22PM -0700, 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)
> 
> This happens to work because the layout of 'struct kobj_attribute' and
> 'struct device_attribute' are the same, so the container_of() cast
> happens to allow the callbacks to still work.

Please note that this was an explicit design decision all those years
ago, it's not just "happening" to work by some accident.  It was just
done way before anyone thought of CFI-like things.

> 
> 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>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/acpi/platform_profile.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index d2f7fd7743a1..11278f785526 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -22,8 +22,8 @@ static const char * const profile_names[] = {
>  };
>  static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>  
> -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)
>  {
>  	int len = 0;
> @@ -49,8 +49,8 @@ static ssize_t platform_profile_choices_show(struct device *dev,
>  	return len;
>  }
>  
> -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_BALANCED;
> @@ -77,8 +77,8 @@ static ssize_t platform_profile_show(struct device *dev,
>  	return sysfs_emit(buf, "%s\n", profile_names[profile]);
>  }
>  
> -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)
>  {
>  	int err, i;
> @@ -115,12 +115,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);

I understand your need/want for this, but ick, is there any way to get
back to using 'struct device' and not "raw" kobjects here?  That's what
the code should be using really.

thanks,

greg k-h