* [PATCH 1/2] ACPI: bgrt: Fix CFI violation
@ 2021-06-23 1:38 Nathan Chancellor
2021-06-23 1:38 ` [PATCH 2/2] ACPI: bgrt: Use sysfs_emit Nathan Chancellor
2021-06-23 16:32 ` [PATCH 1/2] ACPI: bgrt: Fix CFI violation Kees Cook
0 siblings, 2 replies; 7+ messages in thread
From: Nathan Chancellor @ 2021-06-23 1:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, Nick Desaulniers, Sami Tolvanen,
Kees Cook, clang-built-linux, Nathan Chancellor
clang's Control Flow Integrity requires that every indirect call has a
valid target, which is based on the type of the function pointer. The
*_show() functions in this file are written as if they will be called
from dev_attr_show(); however, they will be called from
sysfs_kf_seq_show() because the files were created by
sysfs_create_group() and the sysfs ops are based on kobj_sysfs_ops
because of kobject_add_and_create(). Because the *_show() functions do
not match the type of the show() member in struct kobj_attribute, there
is a CFI violation.
$ cat /sys/firmware/acpi/bgrt/{status,type,version,{x,y}offset}}
1
0
1
522
307
$ dmesg | grep "CFI failure"
[ 267.761825] CFI failure (target: type_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
[ 267.762246] CFI failure (target: xoffset_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
[ 267.762584] CFI failure (target: status_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
[ 267.762973] CFI failure (target: yoffset_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
[ 267.763330] CFI failure (target: version_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
Convert these functions to the type of the show() member in struct
kobj_attribute so that there is no more CFI violation. Because these
functions are all so similar, combine them into a macro.
Fixes: d1ff4b1cdbab ("ACPI: Add support for exposing BGRT data")
Link: https://github.com/ClangBuiltLinux/linux/issues/1406
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
drivers/acpi/bgrt.c | 57 ++++++++++++++-------------------------------
1 file changed, 18 insertions(+), 39 deletions(-)
diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
index 19bb7f870204..e0d14017706e 100644
--- a/drivers/acpi/bgrt.c
+++ b/drivers/acpi/bgrt.c
@@ -15,40 +15,19 @@
static void *bgrt_image;
static struct kobject *bgrt_kobj;
-static ssize_t version_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
-}
-static DEVICE_ATTR_RO(version);
-
-static ssize_t status_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
-}
-static DEVICE_ATTR_RO(status);
-
-static ssize_t type_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
-}
-static DEVICE_ATTR_RO(type);
-
-static ssize_t xoffset_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
-}
-static DEVICE_ATTR_RO(xoffset);
-
-static ssize_t yoffset_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
-}
-static DEVICE_ATTR_RO(yoffset);
+#define BGRT_SHOW(_name, _member) \
+ static ssize_t _name##_show(struct kobject *kobj, \
+ struct kobj_attribute *attr, char *buf) \
+ { \
+ return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab._member); \
+ } \
+ struct kobj_attribute bgrt_attr_##_name = __ATTR_RO(_name)
+
+BGRT_SHOW(version, version);
+BGRT_SHOW(status, status);
+BGRT_SHOW(type, image_type);
+BGRT_SHOW(xoffset, image_offset_x);
+BGRT_SHOW(yoffset, image_offset_y);
static ssize_t image_read(struct file *file, struct kobject *kobj,
struct bin_attribute *attr, char *buf, loff_t off, size_t count)
@@ -60,11 +39,11 @@ static ssize_t image_read(struct file *file, struct kobject *kobj,
static BIN_ATTR_RO(image, 0); /* size gets filled in later */
static struct attribute *bgrt_attributes[] = {
- &dev_attr_version.attr,
- &dev_attr_status.attr,
- &dev_attr_type.attr,
- &dev_attr_xoffset.attr,
- &dev_attr_yoffset.attr,
+ &bgrt_attr_version.attr,
+ &bgrt_attr_status.attr,
+ &bgrt_attr_type.attr,
+ &bgrt_attr_xoffset.attr,
+ &bgrt_attr_yoffset.attr,
NULL,
};
base-commit: a51c80057a887e0f24bd8303b0791a130ff04121
--
2.32.0.93.g670b81a890
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] ACPI: bgrt: Use sysfs_emit
2021-06-23 1:38 [PATCH 1/2] ACPI: bgrt: Fix CFI violation Nathan Chancellor
@ 2021-06-23 1:38 ` Nathan Chancellor
2021-06-23 5:51 ` Kees Cook
2021-06-23 16:32 ` [PATCH 1/2] ACPI: bgrt: Fix CFI violation Kees Cook
1 sibling, 1 reply; 7+ messages in thread
From: Nathan Chancellor @ 2021-06-23 1:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, Nick Desaulniers, Sami Tolvanen,
Kees Cook, clang-built-linux, Nathan Chancellor
sysfs_emit is preferred to snprintf for emitting values after
commit 2efc459d06f1 ("sysfs: Add sysfs_emit and sysfs_emit_at to format
sysfs output").
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
drivers/acpi/bgrt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
index e0d14017706e..02d208732f9a 100644
--- a/drivers/acpi/bgrt.c
+++ b/drivers/acpi/bgrt.c
@@ -19,7 +19,7 @@ static struct kobject *bgrt_kobj;
static ssize_t _name##_show(struct kobject *kobj, \
struct kobj_attribute *attr, char *buf) \
{ \
- return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab._member); \
+ return sysfs_emit(buf, "%d\n", bgrt_tab._member); \
} \
struct kobj_attribute bgrt_attr_##_name = __ATTR_RO(_name)
--
2.32.0.93.g670b81a890
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ACPI: bgrt: Use sysfs_emit
2021-06-23 1:38 ` [PATCH 2/2] ACPI: bgrt: Use sysfs_emit Nathan Chancellor
@ 2021-06-23 5:51 ` Kees Cook
2021-06-23 16:28 ` Nathan Chancellor
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-06-23 5:51 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
Nick Desaulniers, Sami Tolvanen, clang-built-linux
On Tue, Jun 22, 2021 at 06:38:02PM -0700, Nathan Chancellor wrote:
> sysfs_emit is preferred to snprintf for emitting values after
> commit 2efc459d06f1 ("sysfs: Add sysfs_emit and sysfs_emit_at to format
> sysfs output").
>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Perhaps just squash this into patch 1? Looks good otherwise!
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ACPI: bgrt: Use sysfs_emit
2021-06-23 5:51 ` Kees Cook
@ 2021-06-23 16:28 ` Nathan Chancellor
2021-06-23 16:32 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Chancellor @ 2021-06-23 16:28 UTC (permalink / raw)
To: Kees Cook
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
Nick Desaulniers, Sami Tolvanen, clang-built-linux
On 6/22/2021 10:51 PM, Kees Cook wrote:
> On Tue, Jun 22, 2021 at 06:38:02PM -0700, Nathan Chancellor wrote:
>> sysfs_emit is preferred to snprintf for emitting values after
>> commit 2efc459d06f1 ("sysfs: Add sysfs_emit and sysfs_emit_at to format
>> sysfs output").
>>
>> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
>
> Perhaps just squash this into patch 1? Looks good otherwise!
>
I thought about it but sysfs_emit is a relatively new API and the
previous change may want to be backported but I do not have a strong
opinion so I can squash it if Rafael or Len feel strongly :)
Thanks for taking a look, cheers!
Nathan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ACPI: bgrt: Use sysfs_emit
2021-06-23 16:28 ` Nathan Chancellor
@ 2021-06-23 16:32 ` Kees Cook
2021-06-23 17:29 ` Rafael J. Wysocki
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-06-23 16:32 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
Nick Desaulniers, Sami Tolvanen, clang-built-linux
On Wed, Jun 23, 2021 at 09:28:55AM -0700, Nathan Chancellor wrote:
> On 6/22/2021 10:51 PM, Kees Cook wrote:
> > On Tue, Jun 22, 2021 at 06:38:02PM -0700, Nathan Chancellor wrote:
> > > sysfs_emit is preferred to snprintf for emitting values after
> > > commit 2efc459d06f1 ("sysfs: Add sysfs_emit and sysfs_emit_at to format
> > > sysfs output").
> > >
> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> >
> > Perhaps just squash this into patch 1? Looks good otherwise!
> >
>
> I thought about it but sysfs_emit is a relatively new API and the previous
> change may want to be backported but I do not have a strong opinion so I can
> squash it if Rafael or Len feel strongly :)
Fair enough. :) I figured since CFI is even newer than sysfs_emit(), it
didn't make sense to backport. Regardless:
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ACPI: bgrt: Use sysfs_emit
2021-06-23 16:32 ` Kees Cook
@ 2021-06-23 17:29 ` Rafael J. Wysocki
0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-06-23 17:29 UTC (permalink / raw)
To: Kees Cook, Nathan Chancellor
Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
Linux Kernel Mailing List, Nick Desaulniers, Sami Tolvanen,
clang-built-linux
On Wed, Jun 23, 2021 at 6:32 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jun 23, 2021 at 09:28:55AM -0700, Nathan Chancellor wrote:
> > On 6/22/2021 10:51 PM, Kees Cook wrote:
> > > On Tue, Jun 22, 2021 at 06:38:02PM -0700, Nathan Chancellor wrote:
> > > > sysfs_emit is preferred to snprintf for emitting values after
> > > > commit 2efc459d06f1 ("sysfs: Add sysfs_emit and sysfs_emit_at to format
> > > > sysfs output").
> > > >
> > > > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > >
> > > Perhaps just squash this into patch 1? Looks good otherwise!
> > >
> >
> > I thought about it but sysfs_emit is a relatively new API and the previous
> > change may want to be backported but I do not have a strong opinion so I can
> > squash it if Rafael or Len feel strongly :)
>
> Fair enough. :) I figured since CFI is even newer than sysfs_emit(), it
> didn't make sense to backport. Regardless:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Applied along with the [1/2] as 5.14 material, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] ACPI: bgrt: Fix CFI violation
2021-06-23 1:38 [PATCH 1/2] ACPI: bgrt: Fix CFI violation Nathan Chancellor
2021-06-23 1:38 ` [PATCH 2/2] ACPI: bgrt: Use sysfs_emit Nathan Chancellor
@ 2021-06-23 16:32 ` Kees Cook
1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2021-06-23 16:32 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
Nick Desaulniers, Sami Tolvanen, clang-built-linux
On Tue, Jun 22, 2021 at 06:38:01PM -0700, Nathan Chancellor wrote:
> clang's Control Flow Integrity requires that every indirect call has a
> valid target, which is based on the type of the function pointer. The
> *_show() functions in this file are written as if they will be called
> from dev_attr_show(); however, they will be called from
> sysfs_kf_seq_show() because the files were created by
> sysfs_create_group() and the sysfs ops are based on kobj_sysfs_ops
> because of kobject_add_and_create(). Because the *_show() functions do
> not match the type of the show() member in struct kobj_attribute, there
> is a CFI violation.
>
> $ cat /sys/firmware/acpi/bgrt/{status,type,version,{x,y}offset}}
> 1
> 0
> 1
> 522
> 307
>
> $ dmesg | grep "CFI failure"
> [ 267.761825] CFI failure (target: type_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
> [ 267.762246] CFI failure (target: xoffset_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
> [ 267.762584] CFI failure (target: status_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
> [ 267.762973] CFI failure (target: yoffset_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
> [ 267.763330] CFI failure (target: version_show.d5e1ad21498a5fd14edbc5c320906598.cfi_jt+0x0/0x8):
>
> Convert these functions to the type of the show() member in struct
> kobj_attribute so that there is no more CFI violation. Because these
> functions are all so similar, combine them into a macro.
>
> Fixes: d1ff4b1cdbab ("ACPI: Add support for exposing BGRT data")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1406
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Thanks for solving this!
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-23 17:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 1:38 [PATCH 1/2] ACPI: bgrt: Fix CFI violation Nathan Chancellor
2021-06-23 1:38 ` [PATCH 2/2] ACPI: bgrt: Use sysfs_emit Nathan Chancellor
2021-06-23 5:51 ` Kees Cook
2021-06-23 16:28 ` Nathan Chancellor
2021-06-23 16:32 ` Kees Cook
2021-06-23 17:29 ` Rafael J. Wysocki
2021-06-23 16:32 ` [PATCH 1/2] ACPI: bgrt: Fix CFI violation Kees Cook
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).