| Message ID | 20240819-acpi-platform_profile-fix-cfi-violation-v1-1-479365d848f6@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: platform-profile: Fix CFI violation when accessing sysfs files | expand |
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
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
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
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 };
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(-) --- base-commit: 47ac09b91befbb6a235ab620c32af719f8208399 change-id: 20240819-acpi-platform_profile-fix-cfi-violation-de278753bd5f Best regards,