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

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

[Nathan Chancellor]

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.

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);
 
 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: 47ac09b91befbb6a235ab620c32af719f8208399
change-id: 20240819-acpi-platform_profile-fix-cfi-violation-de278753bd5f

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

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

[Sami Tolvanen]

Hi Nathan,

On Mon, Aug 19, 2024 at 12:09 PM Nathan Chancellor <nathan@kernel.org> 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.
>
> 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(-)

Looks good to me, thanks for fixing this!

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

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

[Greg KH]

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

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

[Nathan Chancellor]

Hi Greg,

On Tue, Aug 20, 2024 at 06:15:52AM +0200, Greg KH wrote:
> 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.

Ack, thanks for the additional context! I can shore up this block with
something like (wording improvements welcome):

  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.

which might sound less accusatory (not my intention). I just remember
getting feedback on a patch similar to this a long time ago (perhaps
from Kees?) around "why does this even work as is if the types are
wrong?".

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

Not sure, I did not write this driver and I am unfamiliar with the
'struct device' infrastructure. I see some other drivers in drivers/acpi
and drivers/firmware that use raw kobjects due to sysfs_create_group()
under firmware_kobj, so this does not necessarily feel out of place. If
I got some hints, I could potentially try to do that conversion and have
John test it with CFI (since I do not have this hardware) but I would
think this could still be necessary for stable depending on how that
conversion turns out.

Cheers,
Nathan