Re: [PATCH] ACPI: sysfs: Fix pm_profile_attr type

[Kees Cook <keescook@chromium.org>]

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: G        W         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:ffffaa6dc3c53d30 EFLAGS: 00010246
> [   17.352578] RAX: 331267e0c06cee00 RBX: ffffffff83d85890 RCX: ffffffff8483a6f8
> [   17.352579] RDX: ffff9cceabbb37c0 RSI: 0000000000000082 RDI: ffffffff84bb9e1c
> [   17.352579] RBP: ffffffff845b2bc8 R08: 0000000000000001 R09: ffff9cceabbba200
> [   17.352579] R10: 000000000000019d R11: 0000000000000000 R12: ffff9cc947766f00
> [   17.352580] R13: ffffffff83d6bd50 R14: ffff9ccc6fa80000 R15: ffffffff845bd328
> [   17.352582] FS:  00007fdbc8d13580(0000) GS:ffff9cce91ac0000(0000) knlGS:0000000000000000
> [   17.352582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   17.352583] CR2: 00007fdbc858e000 CR3: 00000005174d0000 CR4: 0000000000340ea0
> [   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 <byahu140@heisei.be>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  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 attribute        attr;
        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);
};

struct kobj_attribute {
        struct attribute attr;
        ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
                        char *buf);
        ssize_t (*store)(struct kobject *kobj, struct kobj_attribute *attr,
                         const char *buf, size_t count);
};

But the show and store are different prototypes, so surely any variable
assignments or argument passing would catch the mismatch. But no!
The sysfs API only takes the .attr member address:

        result = sysfs_create_file(acpi_kobj, &pm_profile_attr.attr);

and, of course, that doesn't break because both struct device_attribute
and struct kobj_attribute do, in fact, use the same structure for their
.attr (struct attribute).

But here's the kicker:

static ssize_t kobj_attr_show(struct kobject *kobj, struct attribute *attr,
                              char *buf)
{
        struct kobj_attribute *kattr;
        ssize_t ret = -EIO;

        kattr = container_of(attr, struct kobj_attribute, attr);
        if (kattr->show)
                ret = kattr->show(kobj, kattr, buf);
	...

A container_of() is used to calculate the offset. This doesn't explode
(normally) at runtime because, as established, these structures have the
same layout, so .show is in the same place.

Some thoughts that I am terrified to check or attempt, but I can't help
myself:

1) Is __ATTR() regularly used to perform cross-structure initialization?

Answer appears to be "yes":

include/linux/device.h: struct device_attribute dev_attr_##_name = __ATTR_WO(_name)
include/linux/device/bus.h:     struct bus_attribute bus_attr_##_name = __ATTR_RW(_name)

2) Should static initializer macros be typed to catch bad cross-type
   assignments? (Which depends on "1" being "no".)

Changing this looks very hard, but it does make me wonder about doing
stuff like this for static initializer macros:

-#define __ATTR(_name, _mode, _show, _store) { \
+#define __ATTR(_name, _mode, _show, _store) (struct kobject *) { \

Obviously not possible here, though.

3) This cannot possibly be the only case of this. Given the answer to
   #1, this bug must be endemic.

static inline int __must_check sysfs_create_file(struct kobject *kobj,
                                                 const struct attribute *attr)
{
        return sysfs_create_file_ns(kobj, attr, NULL);
}

$ git grep 'sysfs_create_file.*, &.*\.attr' | wc -l
51

16 appear to actually be kobj_attribute. Those are fine.

Similar to the patch above, 9 more are from DEVICE_ATTR() (named
"dev_attr_$foo):

#define DEVICE_ATTR(_name, _mode, _show, _store) \
        struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, _store)

And a here are a bunch more macro-based ones:

class_attr	is struct class_attribute

mdev_type_attr	is struct mdev_type_attribute

format_attr	is half struct device_attribute and half struct kobj_attribute:

arch/x86/events/amd/uncore.c:static struct device_attribute format_attr_##_dev##_name = __ATTR_RO(_dev);
arch/x86/events/intel/cstate.c:static struct kobj_attribute format_attr_##_var =                \
arch/x86/events/intel/uncore.h:static struct kobj_attribute format_attr_##_var =                   \
arch/x86/events/rapl.c:static struct kobj_attribute format_attr_##_var = \
drivers/perf/thunderx2_pmu.c:static struct device_attribute format_attr_##_var =                   \
include/linux/perf_event.h:static struct device_attribute format_attr_##_name = __ATTR_RO(_name)

These 2 are also not kobj_attribute:

include/linux/module.h:extern struct module_attribute module_uevent;
kernel/module.c:struct module_attribute module_uevent =

I think all of these non-kobj_attribute cases will trip CFI too, and
this design pattern appears to be intentional. So that will be fun! :)

I haven't gone through all of the 51 carefully, but this looks like a
much larger problem than just this one place. :(

-- 
Kees Cook