* Re: [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype
2023-01-03 23:46 [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype Kees Cook
@ 2023-01-04 0:12 ` Nathan Chancellor
2023-01-04 0:23 ` Eric Biggers
2023-01-04 0:55 ` Gustavo A. R. Silva
2 siblings, 0 replies; 5+ messages in thread
From: Nathan Chancellor @ 2023-01-04 0:12 UTC (permalink / raw)
To: Kees Cook
Cc: tytso, Andreas Dilger, Nick Desaulniers, Tom Rix, linux-kernel,
linux-ext4, llvm, linux-hardening
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype
2023-01-03 23:46 [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype Kees Cook
2023-01-04 0:12 ` Nathan Chancellor
@ 2023-01-04 0:23 ` Eric Biggers
2023-01-04 6:43 ` Eric Biggers
2023-01-04 0:55 ` Gustavo A. R. Silva
2 siblings, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2023-01-04 0:23 UTC (permalink / raw)
To: Kees Cook
Cc: tytso, Andreas Dilger, Nathan Chancellor, Nick Desaulniers,
Tom Rix, linux-kernel, linux-ext4, llvm, linux-hardening
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype
2023-01-04 0:23 ` Eric Biggers
@ 2023-01-04 6:43 ` Eric Biggers
0 siblings, 0 replies; 5+ messages in thread
From: Eric Biggers @ 2023-01-04 6:43 UTC (permalink / raw)
To: Kees Cook
Cc: tytso, Andreas Dilger, Nathan Chancellor, Nick Desaulniers,
Tom Rix, linux-kernel, linux-ext4, llvm, linux-hardening
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype
2023-01-03 23:46 [PATCH] ext4: Fix function prototype mismatch for ext4_feat_ktype Kees Cook
2023-01-04 0:12 ` Nathan Chancellor
2023-01-04 0:23 ` Eric Biggers
@ 2023-01-04 0:55 ` Gustavo A. R. Silva
2 siblings, 0 replies; 5+ messages in thread
From: Gustavo A. R. Silva @ 2023-01-04 0:55 UTC (permalink / raw)
To: Kees Cook
Cc: tytso, Andreas Dilger, Nathan Chancellor, Nick Desaulniers,
Tom Rix, linux-kernel, linux-ext4, llvm, linux-hardening
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread