diff mbox series

binfmt_misc: fix shift-out-of-bounds in check_special_flags

Message ID 20221102025123.1117184-1-liushixin2@huawei.com
State New
Headers
Series binfmt_misc: fix shift-out-of-bounds in check_special_flags |

Commit Message

Liu Shixin Nov. 2, 2022, 2:51 a.m. UTC 
  UBSAN reported a shift-out-of-bounds warning:

 left shift of 1 by 31 places cannot be represented in type 'int'
 Call Trace:
  <TASK>
  __dump_stack lib/dump_stack.c:88 [inline]
  dump_stack_lvl+0x8d/0xcf lib/dump_stack.c:106
  ubsan_epilogue+0xa/0x44 lib/ubsan.c:151
  __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 lib/ubsan.c:322
  check_special_flags fs/binfmt_misc.c:241 [inline]
  create_entry fs/binfmt_misc.c:456 [inline]
  bm_register_write+0x9d3/0xa20 fs/binfmt_misc.c:654
  vfs_write+0x11e/0x580 fs/read_write.c:582
  ksys_write+0xcf/0x120 fs/read_write.c:637
  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
  do_syscall_64+0x34/0x80 arch/x86/entry/common.c:80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd
 RIP: 0033:0x4194e1

Since the type of Node's flags is unsigned long, we should define these
macros with same type too.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 fs/binfmt_misc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Al Viro Nov. 19, 2022, 5:29 a.m. UTC | #1 
On Wed, Nov 02, 2022 at 10:51:23AM +0800, Liu Shixin wrote:
> UBSAN reported a shift-out-of-bounds warning:
> 
>  left shift of 1 by 31 places cannot be represented in type 'int'
>  Call Trace:
>   <TASK>
>   __dump_stack lib/dump_stack.c:88 [inline]
>   dump_stack_lvl+0x8d/0xcf lib/dump_stack.c:106
>   ubsan_epilogue+0xa/0x44 lib/ubsan.c:151
>   __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 lib/ubsan.c:322
>   check_special_flags fs/binfmt_misc.c:241 [inline]
>   create_entry fs/binfmt_misc.c:456 [inline]
>   bm_register_write+0x9d3/0xa20 fs/binfmt_misc.c:654
>   vfs_write+0x11e/0x580 fs/read_write.c:582
>   ksys_write+0xcf/0x120 fs/read_write.c:637
>   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>   do_syscall_64+0x34/0x80 arch/x86/entry/common.c:80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x4194e1
> 
> Since the type of Node's flags is unsigned long, we should define these
> macros with same type too.

We are limited to 32 bits anyway.  More interesting question here is what's
the point of having those bits that high anyway?
Kees Cook Dec. 2, 2022, 9:55 p.m. UTC | #2 
On Sat, Nov 19, 2022 at 05:29:22AM +0000, Al Viro wrote:
> On Wed, Nov 02, 2022 at 10:51:23AM +0800, Liu Shixin wrote:
> > UBSAN reported a shift-out-of-bounds warning:
> > 
> >  left shift of 1 by 31 places cannot be represented in type 'int'
> >  Call Trace:
> >   <TASK>
> >   __dump_stack lib/dump_stack.c:88 [inline]
> >   dump_stack_lvl+0x8d/0xcf lib/dump_stack.c:106
> >   ubsan_epilogue+0xa/0x44 lib/ubsan.c:151
> >   __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 lib/ubsan.c:322
> >   check_special_flags fs/binfmt_misc.c:241 [inline]
> >   create_entry fs/binfmt_misc.c:456 [inline]
> >   bm_register_write+0x9d3/0xa20 fs/binfmt_misc.c:654
> >   vfs_write+0x11e/0x580 fs/read_write.c:582
> >   ksys_write+0xcf/0x120 fs/read_write.c:637
> >   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >   do_syscall_64+0x34/0x80 arch/x86/entry/common.c:80
> >   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >  RIP: 0033:0x4194e1
> > 
> > Since the type of Node's flags is unsigned long, we should define these
> > macros with same type too.
> 
> We are limited to 32 bits anyway.  More interesting question here is what's
> the point of having those bits that high anyway?

Hm, looks like it was designed to avoid the enum just before the
defines:

enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
#define MISC_FMT_CREDENTIALS (1 << 29)
#define MISC_FMT_OPEN_FILE (1 << 28)

But both appear to be entirely internally defined bits. I think these
can all just be part of the enum, and we can quit mixing "&" tests and
test_bit() calls:

...
        if (e->flags & MISC_FMT_OPEN_FILE)
                *dp++ = 'F';
        *dp++ = '\n';

        if (!test_bit(Magic, &e->flags)) {
                sprintf(dp, "extension .%s\n", e->magic);
...
Kees Cook Dec. 2, 2022, 9:58 p.m. UTC | #3 
On Wed, 2 Nov 2022 10:51:23 +0800, Liu Shixin wrote:
> UBSAN reported a shift-out-of-bounds warning:
> 
>  left shift of 1 by 31 places cannot be represented in type 'int'
>  Call Trace:
>   <TASK>
>   __dump_stack lib/dump_stack.c:88 [inline]
>   dump_stack_lvl+0x8d/0xcf lib/dump_stack.c:106
>   ubsan_epilogue+0xa/0x44 lib/ubsan.c:151
>   __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 lib/ubsan.c:322
>   check_special_flags fs/binfmt_misc.c:241 [inline]
>   create_entry fs/binfmt_misc.c:456 [inline]
>   bm_register_write+0x9d3/0xa20 fs/binfmt_misc.c:654
>   vfs_write+0x11e/0x580 fs/read_write.c:582
>   ksys_write+0xcf/0x120 fs/read_write.c:637
>   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>   do_syscall_64+0x34/0x80 arch/x86/entry/common.c:80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x4194e1
> 
> [...]

Applied to for-next/execve, thanks!

[1/1] binfmt_misc: fix shift-out-of-bounds in check_special_flags
      https://git.kernel.org/kees/c/6a46bf558803
diff mbox series

Patch

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index e1eae7ea823a..bb202ad369d5 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -44,10 +44,10 @@  static LIST_HEAD(entries);
 static int enabled = 1;
 
 enum {Enabled, Magic};
-#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
-#define MISC_FMT_OPEN_BINARY (1 << 30)
-#define MISC_FMT_CREDENTIALS (1 << 29)
-#define MISC_FMT_OPEN_FILE (1 << 28)
+#define MISC_FMT_PRESERVE_ARGV0 (1UL << 31)
+#define MISC_FMT_OPEN_BINARY (1UL << 30)
+#define MISC_FMT_CREDENTIALS (1UL << 29)
+#define MISC_FMT_OPEN_FILE (1UL << 28)
 
 typedef struct {
 	struct list_head list;