Re: [PATCH] ACPI: sysfs: Fix pm_profile_attr type
On Friday, June 12, 2020 6:51:50 AM CEST Nathan Chancellor wrote:
> When running a kernel with Clang's Control Flow Integrity implemented,
> there is a violation that happens when accessing
> /sys/firmware/acpi/pm_profile:
>
> $ cat /sys/firmware/acpi/pm_profile
> 0
>
> $ dmesg
> ...
> [ 17.352564] [ cut here ]
> [ 17.352568] CFI failure (target: acpi_show_profile+0x0/0x8):
> [ 17.352572] WARNING: CPU: 3 PID: 497 at kernel/cfi.c:29
> __cfi_check_fail+0x33/0x40
> [ 17.352573] Modules linked in:
> [ 17.352575] CPU: 3 PID: 497 Comm: cat Tainted: GW
> 5.7.0-microsoft-standard+ #1
> [ 17.352576] RIP: 0010:__cfi_check_fail+0x33/0x40
> [ 17.352577] Code: 48 c7 c7 50 b3 85 84 48 c7 c6 50 0a 4e 84 e8 a4 d8 60 00
> 85 c0 75 02 5b c3 48 c7 c7 dc 5e 49 84 48 89 de 31 c0 e8 7d 06 eb ff <0f> 0b
> 5b c3 00 00 cc cc 00 00 cc cc 00 85 f6 74 25 41 b9 ea ff ff
> [ 17.352577] RSP: 0018:aa6dc3c53d30 EFLAGS: 00010246
> [ 17.352578] RAX: 331267e0c06cee00 RBX: 83d85890 RCX:
> 8483a6f8
> [ 17.352579] RDX: 9cceabbb37c0 RSI: 0082 RDI:
> 84bb9e1c
> [ 17.352579] RBP: 845b2bc8 R08: 0001 R09:
> 9cceabbba200
> [ 17.352579] R10: 019d R11: R12:
> 9cc947766f00
> [ 17.352580] R13: 83d6bd50 R14: 9ccc6fa8 R15:
> 845bd328
> [ 17.352582] FS: 7fdbc8d13580() GS:9cce91ac()
> knlGS:
> [ 17.352582] CS: 0010 DS: ES: CR0: 80050033
> [ 17.352583] CR2: 7fdbc858e000 CR3: 0005174d CR4:
> 00340ea0
> [ 17.352584] Call Trace:
> [ 17.352586] ? rev_id_show+0x8/0x8
> [ 17.352587] ? __cfi_check+0x45bac/0x4b640
> [ 17.352589] ? kobj_attr_show+0x73/0x80
> [ 17.352590] ? sysfs_kf_seq_show+0xc1/0x140
> [ 17.352592] ? ext4_seq_options_show.cfi_jt+0x8/0x8
> [ 17.352593] ? seq_read+0x180/0x600
> [ 17.352595] ? sysfs_create_file_ns.cfi_jt+0x10/0x10
> [ 17.352596] ? tlbflush_read_file+0x8/0x8
> [ 17.352597] ? __vfs_read+0x6b/0x220
> [ 17.352598] ? handle_mm_fault+0xa23/0x11b0
> [ 17.352599] ? vfs_read+0xa2/0x130
> [ 17.352599] ? ksys_read+0x6a/0xd0
> [ 17.352601] ? __do_sys_getpgrp+0x8/0x8
> [ 17.352602] ? do_syscall_64+0x72/0x120
> [ 17.352603] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 17.352604] ---[ end trace 7b1fa81dc897e419 ]---
>
> When /sys/firmware/acpi/pm_profile is read, sysfs_kf_seq_show is called,
> which in turn calls kobj_attr_show, which gets the ->show callback
> member by calling container_of on attr (casting it to struct
> kobj_attribute) then calls it.
>
> There is a CFI violation because pm_profile_attr is of type
> struct device_attribute but kobj_attr_show calls ->show expecting it
> to be from struct kobj_attribute. CFI checking ensures that function
> pointer types match when doing indirect calls. Fix pm_profile_attr to
> be defined in terms of kobj_attribute so there is no violation or
> mismatch.
>
> Fixes: 362b646062b2 ("ACPI: Export FADT pm_profile integer value to
> userspace")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1051
> Reported-by: yuu ichii
> Signed-off-by: Nathan Chancellor
> ---
> drivers/acpi/sysfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 3a89909b50a6..76c668c05fa0 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -938,13 +938,13 @@ static void __exit interrupt_stats_exit(void)
> }
>
> static ssize_t
> -acpi_show_profile(struct device *dev, struct device_attribute *attr,
> +acpi_show_profile(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> return sprintf(buf, "%d\n", acpi_gbl_FADT.preferred_profile);
> }
>
> -static const struct device_attribute pm_profile_attr =
> +static const struct kobj_attribute pm_profile_attr =
> __ATTR(pm_profile, S_IRUGO, acpi_show_profile, NULL);
>
> static ssize_t hotplug_enabled_show(struct kobject *kobj,
>
> base-commit: b791d1bdf9212d944d749a5c7ff6febdba241771
>
Applied as 5.8-rc material, thanks!
Re: [PATCH] ACPI: sysfs: Fix pm_profile_attr type
On Thu, Jun 11, 2020 at 11:17:14PM -0700, Kees Cook wrote:
> On Thu, Jun 11, 2020 at 09:51:50PM -0700, Nathan Chancellor wrote:
> > When running a kernel with Clang's Control Flow Integrity implemented,
> > there is a violation that happens when accessing
> > /sys/firmware/acpi/pm_profile:
> >
> > $ cat /sys/firmware/acpi/pm_profile
> > 0
> >
> > $ dmesg
> > ...
> > [ 17.352564] [ cut here ]
> > [ 17.352568] CFI failure (target: acpi_show_profile+0x0/0x8):
> > [ 17.352572] WARNING: CPU: 3 PID: 497 at kernel/cfi.c:29
> > __cfi_check_fail+0x33/0x40
> > [ 17.352573] Modules linked in:
> > [ 17.352575] CPU: 3 PID: 497 Comm: cat Tainted: GW
> > 5.7.0-microsoft-standard+ #1
> > [ 17.352576] RIP: 0010:__cfi_check_fail+0x33/0x40
> > [ 17.352577] Code: 48 c7 c7 50 b3 85 84 48 c7 c6 50 0a 4e 84 e8 a4 d8 60
> > 00 85 c0 75 02 5b c3 48 c7 c7 dc 5e 49 84 48 89 de 31 c0 e8 7d 06 eb ff
> > <0f> 0b 5b c3 00 00 cc cc 00 00 cc cc 00 85 f6 74 25 41 b9 ea ff ff
> > [ 17.352577] RSP: 0018:aa6dc3c53d30 EFLAGS: 00010246
> > [ 17.352578] RAX: 331267e0c06cee00 RBX: 83d85890 RCX:
> > 8483a6f8
> > [ 17.352579] RDX: 9cceabbb37c0 RSI: 0082 RDI:
> > 84bb9e1c
> > [ 17.352579] RBP: 845b2bc8 R08: 0001 R09:
> > 9cceabbba200
> > [ 17.352579] R10: 019d R11: R12:
> > 9cc947766f00
> > [ 17.352580] R13: 83d6bd50 R14: 9ccc6fa8 R15:
> > 845bd328
> > [ 17.352582] FS: 7fdbc8d13580() GS:9cce91ac()
> > knlGS:
> > [ 17.352582] CS: 0010 DS: ES: CR0: 80050033
> > [ 17.352583] CR2: 7fdbc858e000 CR3: 0005174d CR4:
> > 00340ea0
> > [ 17.352584] Call Trace:
> > [ 17.352586] ? rev_id_show+0x8/0x8
> > [ 17.352587] ? __cfi_check+0x45bac/0x4b640
> > [ 17.352589] ? kobj_attr_show+0x73/0x80
> > [ 17.352590] ? sysfs_kf_seq_show+0xc1/0x140
> > [ 17.352592] ? ext4_seq_options_show.cfi_jt+0x8/0x8
> > [ 17.352593] ? seq_read+0x180/0x600
> > [ 17.352595] ? sysfs_create_file_ns.cfi_jt+0x10/0x10
> > [ 17.352596] ? tlbflush_read_file+0x8/0x8
> > [ 17.352597] ? __vfs_read+0x6b/0x220
> > [ 17.352598] ? handle_mm_fault+0xa23/0x11b0
> > [ 17.352599] ? vfs_read+0xa2/0x130
> > [ 17.352599] ? ksys_read+0x6a/0xd0
> > [ 17.352601] ? __do_sys_getpgrp+0x8/0x8
> > [ 17.352602] ? do_syscall_64+0x72/0x120
> > [ 17.352603] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 17.352604] ---[ end trace 7b1fa81dc897e419 ]---
> >
> > When /sys/firmware/acpi/pm_profile is read, sysfs_kf_seq_show is called,
> > which in turn calls kobj_attr_show, which gets the ->show callback
> > member by calling container_of on attr (casting it to struct
> > kobj_attribute) then calls it.
> >
> > There is a CFI violation because pm_profile_attr is of type
> > struct device_attribute but kobj_attr_show calls ->show expecting it
> > to be from struct kobj_attribute. CFI checking ensures that function
> > pointer types match when doing indirect calls. Fix pm_profile_attr to
> > be defined in terms of kobj_attribute so there is no violation or
> > mismatch.
> >
> > Fixes: 362b646062b2 ("ACPI: Export FADT pm_profile integer value to
> > userspace")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1051
> > Reported-by: yuu ichii
> > Signed-off-by: Nathan Chancellor
> > ---
> > drivers/acpi/sysfs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> > index 3a89909b50a6..76c668c05fa0 100644
> > --- a/drivers/acpi/sysfs.c
> > +++ b/drivers/acpi/sysfs.c
> > @@ -938,13 +938,13 @@ static void __exit interrupt_stats_exit(void)
> > }
> >
> > static ssize_t
> > -acpi_show_profile(struct device *dev, struct device_attribute *attr,
> > +acpi_show_profile(struct kobject *kobj, struct kobj_attribute *attr,
> > char *buf)
> > {
> > return sprintf(buf, "%d\n", acpi_gbl_FADT.preferred_profile);
> > }
> >
> > -static const struct device_attribute pm_profile_attr =
> > +static const struct kobj_attribute pm_profile_attr =
> > __ATTR(pm_profile, S_IRUGO, acpi_show_profile, NULL);
>
> My mind absolutely rebelled at how this could not have been caught
> at compile time nor runtime already. Everything appears to be wrong
> here. It's a different structure, it's getting assigned, it's getting
> called! And then I went looking and started to scream. Apologies if this
> investigation is redundant to a thread I didn't see...
Yes, I was rather shocked as well... There was no prior discussion to
this patch, just something that was reported to me (the initial
reproducer was 'grep -irl uname /sys').
> First, __ATTR(), like most static initializer macros, is not typed.
> Normally this is okay because different structures have different
> members, so it wouldn't compile. But not in this case
Re: [PATCH] ACPI: sysfs: Fix pm_profile_attr type
On Thu, Jun 11, 2020 at 09:51:50PM -0700, Nathan Chancellor wrote:
> When running a kernel with Clang's Control Flow Integrity implemented,
> there is a violation that happens when accessing
> /sys/firmware/acpi/pm_profile:
>
> $ cat /sys/firmware/acpi/pm_profile
> 0
>
> $ dmesg
> ...
> [ 17.352564] [ cut here ]
> [ 17.352568] CFI failure (target: acpi_show_profile+0x0/0x8):
> [ 17.352572] WARNING: CPU: 3 PID: 497 at kernel/cfi.c:29
> __cfi_check_fail+0x33/0x40
> [ 17.352573] Modules linked in:
> [ 17.352575] CPU: 3 PID: 497 Comm: cat Tainted: GW
> 5.7.0-microsoft-standard+ #1
> [ 17.352576] RIP: 0010:__cfi_check_fail+0x33/0x40
> [ 17.352577] Code: 48 c7 c7 50 b3 85 84 48 c7 c6 50 0a 4e 84 e8 a4 d8 60 00
> 85 c0 75 02 5b c3 48 c7 c7 dc 5e 49 84 48 89 de 31 c0 e8 7d 06 eb ff <0f> 0b
> 5b c3 00 00 cc cc 00 00 cc cc 00 85 f6 74 25 41 b9 ea ff ff
> [ 17.352577] RSP: 0018:aa6dc3c53d30 EFLAGS: 00010246
> [ 17.352578] RAX: 331267e0c06cee00 RBX: 83d85890 RCX:
> 8483a6f8
> [ 17.352579] RDX: 9cceabbb37c0 RSI: 0082 RDI:
> 84bb9e1c
> [ 17.352579] RBP: 845b2bc8 R08: 0001 R09:
> 9cceabbba200
> [ 17.352579] R10: 019d R11: R12:
> 9cc947766f00
> [ 17.352580] R13: 83d6bd50 R14: 9ccc6fa8 R15:
> 845bd328
> [ 17.352582] FS: 7fdbc8d13580() GS:9cce91ac()
> knlGS:
> [ 17.352582] CS: 0010 DS: ES: CR0: 80050033
> [ 17.352583] CR2: 7fdbc858e000 CR3: 0005174d CR4:
> 00340ea0
> [ 17.352584] Call Trace:
> [ 17.352586] ? rev_id_show+0x8/0x8
> [ 17.352587] ? __cfi_check+0x45bac/0x4b640
> [ 17.352589] ? kobj_attr_show+0x73/0x80
> [ 17.352590] ? sysfs_kf_seq_show+0xc1/0x140
> [ 17.352592] ? ext4_seq_options_show.cfi_jt+0x8/0x8
> [ 17.352593] ? seq_read+0x180/0x600
> [ 17.352595] ? sysfs_create_file_ns.cfi_jt+0x10/0x10
> [ 17.352596] ? tlbflush_read_file+0x8/0x8
> [ 17.352597] ? __vfs_read+0x6b/0x220
> [ 17.352598] ? handle_mm_fault+0xa23/0x11b0
> [ 17.352599] ? vfs_read+0xa2/0x130
> [ 17.352599] ? ksys_read+0x6a/0xd0
> [ 17.352601] ? __do_sys_getpgrp+0x8/0x8
> [ 17.352602] ? do_syscall_64+0x72/0x120
> [ 17.352603] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 17.352604] ---[ end trace 7b1fa81dc897e419 ]---
>
> When /sys/firmware/acpi/pm_profile is read, sysfs_kf_seq_show is called,
> which in turn calls kobj_attr_show, which gets the ->show callback
> member by calling container_of on attr (casting it to struct
> kobj_attribute) then calls it.
>
> There is a CFI violation because pm_profile_attr is of type
> struct device_attribute but kobj_attr_show calls ->show expecting it
> to be from struct kobj_attribute. CFI checking ensures that function
> pointer types match when doing indirect calls. Fix pm_profile_attr to
> be defined in terms of kobj_attribute so there is no violation or
> mismatch.
>
> Fixes: 362b646062b2 ("ACPI: Export FADT pm_profile integer value to
> userspace")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1051
> Reported-by: yuu ichii
> Signed-off-by: Nathan Chancellor
> ---
> drivers/acpi/sysfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 3a89909b50a6..76c668c05fa0 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -938,13 +938,13 @@ static void __exit interrupt_stats_exit(void)
> }
>
> static ssize_t
> -acpi_show_profile(struct device *dev, struct device_attribute *attr,
> +acpi_show_profile(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> return sprintf(buf, "%d\n", acpi_gbl_FADT.preferred_profile);
> }
>
> -static const struct device_attribute pm_profile_attr =
> +static const struct kobj_attribute pm_profile_attr =
> __ATTR(pm_profile, S_IRUGO, acpi_show_profile, NULL);
My mind absolutely rebelled at how this could not have been caught
at compile time nor runtime already. Everything appears to be wrong
here. It's a different structure, it's getting assigned, it's getting
called! And then I went looking and started to scream. Apologies if this
investigation is redundant to a thread I didn't see...
First, __ATTR(), like most static initializer macros, is not typed.
Normally this is okay because different structures have different
members, so it wouldn't compile. But not in this case here. Everything
assigned by __ATTR exists in both because ... they have an identical set
of structure member names:
struct device_attribute {
struct attributeattr;
ssize_t (*show)(struct device *dev, struct device_attribute *attr,
char *buf);
ssize_t (*store)(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
};
[PATCH] ACPI: sysfs: Fix pm_profile_attr type
When running a kernel with Clang's Control Flow Integrity implemented,
there is a violation that happens when accessing
/sys/firmware/acpi/pm_profile:
$ cat /sys/firmware/acpi/pm_profile
0
$ dmesg
...
[ 17.352564] [ cut here ]
[ 17.352568] CFI failure (target: acpi_show_profile+0x0/0x8):
[ 17.352572] WARNING: CPU: 3 PID: 497 at kernel/cfi.c:29
__cfi_check_fail+0x33/0x40
[ 17.352573] Modules linked in:
[ 17.352575] CPU: 3 PID: 497 Comm: cat Tainted: GW
5.7.0-microsoft-standard+ #1
[ 17.352576] RIP: 0010:__cfi_check_fail+0x33/0x40
[ 17.352577] Code: 48 c7 c7 50 b3 85 84 48 c7 c6 50 0a 4e 84 e8 a4 d8 60 00
85 c0 75 02 5b c3 48 c7 c7 dc 5e 49 84 48 89 de 31 c0 e8 7d 06 eb ff <0f> 0b 5b
c3 00 00 cc cc 00 00 cc cc 00 85 f6 74 25 41 b9 ea ff ff
[ 17.352577] RSP: 0018:aa6dc3c53d30 EFLAGS: 00010246
[ 17.352578] RAX: 331267e0c06cee00 RBX: 83d85890 RCX: 8483a6f8
[ 17.352579] RDX: 9cceabbb37c0 RSI: 0082 RDI: 84bb9e1c
[ 17.352579] RBP: 845b2bc8 R08: 0001 R09: 9cceabbba200
[ 17.352579] R10: 019d R11: R12: 9cc947766f00
[ 17.352580] R13: 83d6bd50 R14: 9ccc6fa8 R15: 845bd328
[ 17.352582] FS: 7fdbc8d13580() GS:9cce91ac()
knlGS:
[ 17.352582] CS: 0010 DS: ES: CR0: 80050033
[ 17.352583] CR2: 7fdbc858e000 CR3: 0005174d CR4: 00340ea0
[ 17.352584] Call Trace:
[ 17.352586] ? rev_id_show+0x8/0x8
[ 17.352587] ? __cfi_check+0x45bac/0x4b640
[ 17.352589] ? kobj_attr_show+0x73/0x80
[ 17.352590] ? sysfs_kf_seq_show+0xc1/0x140
[ 17.352592] ? ext4_seq_options_show.cfi_jt+0x8/0x8
[ 17.352593] ? seq_read+0x180/0x600
[ 17.352595] ? sysfs_create_file_ns.cfi_jt+0x10/0x10
[ 17.352596] ? tlbflush_read_file+0x8/0x8
[ 17.352597] ? __vfs_read+0x6b/0x220
[ 17.352598] ? handle_mm_fault+0xa23/0x11b0
[ 17.352599] ? vfs_read+0xa2/0x130
[ 17.352599] ? ksys_read+0x6a/0xd0
[ 17.352601] ? __do_sys_getpgrp+0x8/0x8
[ 17.352602] ? do_syscall_64+0x72/0x120
[ 17.352603] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 17.352604] ---[ end trace 7b1fa81dc897e419 ]---
When /sys/firmware/acpi/pm_profile is read, sysfs_kf_seq_show is called,
which in turn calls kobj_attr_show, which gets the ->show callback
member by calling container_of on attr (casting it to struct
kobj_attribute) then calls it.
There is a CFI violation because pm_profile_attr is of type
struct device_attribute but kobj_attr_show calls ->show expecting it
to be from struct kobj_attribute. CFI checking ensures that function
pointer types match when doing indirect calls. Fix pm_profile_attr to
be defined in terms of kobj_attribute so there is no violation or
mismatch.
Fixes: 362b646062b2 ("ACPI: Export FADT pm_profile integer value to userspace")
Link: https://github.com/ClangBuiltLinux/linux/issues/1051
Reported-by: yuu ichii
Signed-off-by: Nathan Chancellor
---
drivers/acpi/sysfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 3a89909b50a6..76c668c05fa0 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -938,13 +938,13 @@ static void __exit interrupt_stats_exit(void)
}
static ssize_t
-acpi_show_profile(struct device *dev, struct device_attribute *attr,
+acpi_show_profile(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
{
return sprintf(buf, "%d\n", acpi_gbl_FADT.preferred_profile);
}
-static const struct device_attribute pm_profile_attr =
+static const struct kobj_attribute pm_profile_attr =
__ATTR(pm_profile, S_IRUGO, acpi_show_profile, NULL);
static ssize_t hotplug_enabled_show(struct kobject *kobj,
base-commit: b791d1bdf9212d944d749a5c7ff6febdba241771
--
2.27.0
