diff mbox series

ext4: Fix function prototype mismatch for ext4_feat_ktype

Message ID 20230103234616.never.915-kees@kernel.org
State New
Headers
Series ext4: Fix function prototype mismatch for ext4_feat_ktype |

Commit Message

Kees Cook Jan. 3, 2023, 11:46 p.m. UTC 
  With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
indirect call targets are validated against the expected function
pointer prototype to make sure the call target is valid to help mitigate
ROP attacks. If they are not identical, there is a failure at run time,
which manifests as either a kernel panic or thread getting killed.

ext4_feat_ktype was setting the "release" handler to "kfree", which
doesn't have a matching function prototype. Add a simple wrapper
with the correct prototype.

This was found as a result of Clang's new -Wcast-function-type-strict
flag, which is more sensitive than the simpler -Wcast-function-type,
which only checks for type width mismatches.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/ext4/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Nathan Chancellor Jan. 4, 2023, 12:12 a.m. UTC | #1 
On Tue, Jan 03, 2023 at 03:46:20PM -0800, Kees Cook wrote:
> With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
> indirect call targets are validated against the expected function
> pointer prototype to make sure the call target is valid to help mitigate
> ROP attacks. If they are not identical, there is a failure at run time,
> which manifests as either a kernel panic or thread getting killed.
> 
> ext4_feat_ktype was setting the "release" handler to "kfree", which
> doesn't have a matching function prototype. Add a simple wrapper
> with the correct prototype.
> 
> This was found as a result of Clang's new -Wcast-function-type-strict
> flag, which is more sensitive than the simpler -Wcast-function-type,
> which only checks for type width mismatches.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  fs/ext4/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index d233c24ea342..83cf8b5afb54 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -491,6 +491,11 @@ static void ext4_sb_release(struct kobject *kobj)
>  	complete(&sbi->s_kobj_unregister);
>  }
>  
> +static void ext4_kobject_release(struct kobject *kobj)
> +{
> +	kfree(kobj);
> +}
> +
>  static const struct sysfs_ops ext4_attr_ops = {
>  	.show	= ext4_attr_show,
>  	.store	= ext4_attr_store,
> @@ -505,7 +510,7 @@ static struct kobj_type ext4_sb_ktype = {
>  static struct kobj_type ext4_feat_ktype = {
>  	.default_groups = ext4_feat_groups,
>  	.sysfs_ops	= &ext4_attr_ops,
> -	.release	= (void (*)(struct kobject *))kfree,
> +	.release	= ext4_kobject_release,
>  };
>  
>  void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)
> -- 
> 2.34.1
>
Eric Biggers Jan. 4, 2023, 12:23 a.m. UTC | #2 
On Tue, Jan 03, 2023 at 03:46:20PM -0800, Kees Cook wrote:
> With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
> indirect call targets are validated against the expected function
> pointer prototype to make sure the call target is valid to help mitigate
> ROP attacks. If they are not identical, there is a failure at run time,
> which manifests as either a kernel panic or thread getting killed.
> 
> ext4_feat_ktype was setting the "release" handler to "kfree", which
> doesn't have a matching function prototype. Add a simple wrapper
> with the correct prototype.
> 
> This was found as a result of Clang's new -Wcast-function-type-strict
> flag, which is more sensitive than the simpler -Wcast-function-type,
> which only checks for type width mismatches.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/ext4/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index d233c24ea342..83cf8b5afb54 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -491,6 +491,11 @@ static void ext4_sb_release(struct kobject *kobj)
>  	complete(&sbi->s_kobj_unregister);
>  }
>  
> +static void ext4_kobject_release(struct kobject *kobj)
> +{
> +	kfree(kobj);
> +}
> +
>  static const struct sysfs_ops ext4_attr_ops = {
>  	.show	= ext4_attr_show,
>  	.store	= ext4_attr_store,
> @@ -505,7 +510,7 @@ static struct kobj_type ext4_sb_ktype = {
>  static struct kobj_type ext4_feat_ktype = {
>  	.default_groups = ext4_feat_groups,
>  	.sysfs_ops	= &ext4_attr_ops,
> -	.release	= (void (*)(struct kobject *))kfree,
> +	.release	= ext4_kobject_release,

For consistency, maybe call this ext4_feat_release?  So ext4_sb_ktype would have
ext4_sb_release, and ext4_feat_ktype would have ext4_feat_release.

I'm also surprised that this wasn't found earlier.  Is it possible that CFI does
not actually distinguish between the two function prototypes here?

- Eric
Gustavo A. R. Silva Jan. 4, 2023, 12:55 a.m. UTC | #3 
On Tue, Jan 03, 2023 at 03:46:20PM -0800, Kees Cook wrote:
> With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
> indirect call targets are validated against the expected function
> pointer prototype to make sure the call target is valid to help mitigate
> ROP attacks. If they are not identical, there is a failure at run time,
> which manifests as either a kernel panic or thread getting killed.
> 
> ext4_feat_ktype was setting the "release" handler to "kfree", which
> doesn't have a matching function prototype. Add a simple wrapper
> with the correct prototype.
> 
> This was found as a result of Clang's new -Wcast-function-type-strict
> flag, which is more sensitive than the simpler -Wcast-function-type,
> which only checks for type width mismatches.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Just for the record, this is the warning Clang reports without this
patch:

fs/ext4/sysfs.c:508:13: warning: cast from 'void (*)(const void *)' to 'void (*)(struct kobject *)' converts to incompatible function type [-Wcast-function-type-strict]

Thanks
--
Gustavo

> ---
>  fs/ext4/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index d233c24ea342..83cf8b5afb54 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -491,6 +491,11 @@ static void ext4_sb_release(struct kobject *kobj)
>  	complete(&sbi->s_kobj_unregister);
>  }
>  
> +static void ext4_kobject_release(struct kobject *kobj)
> +{
> +	kfree(kobj);
> +}
> +
>  static const struct sysfs_ops ext4_attr_ops = {
>  	.show	= ext4_attr_show,
>  	.store	= ext4_attr_store,
> @@ -505,7 +510,7 @@ static struct kobj_type ext4_sb_ktype = {
>  static struct kobj_type ext4_feat_ktype = {
>  	.default_groups = ext4_feat_groups,
>  	.sysfs_ops	= &ext4_attr_ops,
> -	.release	= (void (*)(struct kobject *))kfree,
> +	.release	= ext4_kobject_release,
>  };
>  
>  void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)
> -- 
> 2.34.1
>
Eric Biggers Jan. 4, 2023, 6:43 a.m. UTC | #4 
On Wed, Jan 04, 2023 at 12:23:44AM +0000, Eric Biggers wrote:
> I'm also surprised that this wasn't found earlier.  Is it possible that CFI does
> not actually distinguish between the two function prototypes here?
> 

It's because this code is only reached when ext4 is a loadable module and it is
being unloaded.  I can reproduce the CFI failure by doing that.

In addition to the naming tweak I requested, can you also add Fixes and Cc
stable tags?

By the way, here's the log from the CFI failure I got:

[   16.412498] CFI failure at kobject_put+0xbb/0x1b0 (target: kfree+0x0/0x180; expected type: 0x7c4aa698)
[   16.413716] invalid opcode: 0000 [#1] PREEMPT SMP
[   16.414299] CPU: 2 PID: 179 Comm: rmmod Tainted: G                T  6.2.0-rc2 #28
[   16.415223] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.1-1-1 04/01/2014
[   16.416383] RIP: 0010:kobject_put+0xbb/0x1b0
[   16.416926] Code: df e8 f9 03 00 00 4d 85 e4 74 24 49 83 3c 24 00 74 1d 0f 1f 44 00 00 48 89 df 4d 8b 1c 24 41 ba 68 59 b5 9
[   16.419199] RSP: 0018:ffffc90000283e20 EFLAGS: 00010a13
[   16.419842] RAX: 0000000000000000 RBX: ffff88800436c9c0 RCX: 0000000000000000
[   16.420719] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88800436c9c0
[   16.421595] RBP: ffffc90000283e40 R08: 0000000000000000 R09: 0000000000000000
[   16.422465] R10: 0000000017461662 R11: ffffffff81216a20 R12: ffffffffa00f9510
[   16.423330] R13: 0000000000000000 R14: ffff88800436ca80 R15: ffff888007643950
[   16.424207] FS:  00007f7bf573f740(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000
[   16.425188] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   16.425814] CR2: 0000561eb661f818 CR3: 00000000074c9000 CR4: 00000000003506e0
[   16.426574] Call Trace:
[   16.426862]  <TASK>
[   16.427098]  ext4_exit_sysfs+0x14/0x60 [ext4]
[   16.427604]  cleanup_module+0x67/0xedb [ext4]
[   16.428099]  __se_sys_delete_module+0x22d/0x3e0
[   16.428593]  ? syscall_enter_from_user_mode+0x2a/0x1d0
[   16.429142]  ? syscall_enter_from_user_mode+0x2a/0x1d0
[   16.429694]  __x64_sys_delete_module+0x11/0x20
[   16.430173]  do_syscall_64+0x53/0xb0
[   16.430562]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   16.431151] RIP: 0033:0x7f7bf584ccfb
[   16.431544] Code: 73 01 c3 48 8b 0d 8d f0 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 8
[   16.433517] RSP: 002b:00007fff7d959068 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[   16.434327] RAX: ffffffffffffffda RBX: 0000561eb6615750 RCX: 00007f7bf584ccfb
[   16.435090] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000561eb66157b8
[   16.435852] RBP: 0000000000000000 R08: 1999999999999999 R09: 0000000000000000
[   16.436617] R10: 00007f7bf58c2ac0 R11: 0000000000000206 R12: 0000000000000000
[   16.437472] R13: 00007fff7d9592d0 R14: 00007fff7d959d80 R15: 0000561eb66152a0
[   16.438307]  </TASK>
[   16.438552] Modules linked in: ext4(-) crc32c_generic mbcache crc16 jbd2
[   16.439321] ---[ end trace 0000000000000000 ]---
[   16.439829] RIP: 0010:kobject_put+0xbb/0x1b0
[   16.440294] Code: df e8 f9 03 00 00 4d 85 e4 74 24 49 83 3c 24 00 74 1d 0f 1f 44 00 00 48 89 df 4d 8b 1c 24 41 ba 68 59 b5 9
[   16.442310] RSP: 0018:ffffc90000283e20 EFLAGS: 00010a13
[   16.442960] RAX: 0000000000000000 RBX: ffff88800436c9c0 RCX: 0000000000000000
[   16.443719] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88800436c9c0
[   16.444472] RBP: ffffc90000283e40 R08: 0000000000000000 R09: 0000000000000000
[   16.445231] R10: 0000000017461662 R11: ffffffff81216a20 R12: ffffffffa00f9510
[   16.446012] R13: 0000000000000000 R14: ffff88800436ca80 R15: ffff888007643950
[   16.446779] FS:  00007f7bf573f740(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000
[   16.447658] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   16.448276] CR2: 0000561eb661f818 CR3: 00000000074c9000 CR4: 00000000003506e0
diff mbox series

Patch

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index d233c24ea342..83cf8b5afb54 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -491,6 +491,11 @@  static void ext4_sb_release(struct kobject *kobj)
 	complete(&sbi->s_kobj_unregister);
 }
 
+static void ext4_kobject_release(struct kobject *kobj)
+{
+	kfree(kobj);
+}
+
 static const struct sysfs_ops ext4_attr_ops = {
 	.show	= ext4_attr_show,
 	.store	= ext4_attr_store,
@@ -505,7 +510,7 @@  static struct kobj_type ext4_sb_ktype = {
 static struct kobj_type ext4_feat_ktype = {
 	.default_groups = ext4_feat_groups,
 	.sysfs_ops	= &ext4_attr_ops,
-	.release	= (void (*)(struct kobject *))kfree,
+	.release	= ext4_kobject_release,
 };
 
 void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)