Re: [PATCH] ext4: silence array overflow warning

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



CC fsdevel, because this bug is duplicated at least in 6 other filesystems:
ext2, exofs, ocfs2, f2fs, nilfs2, btrfs

On Tue, Jul 18, 2017 at 12:27 PM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> wrote:
> I get a static checker warning:
>
>     fs/ext4/ext4.h:3091 ext4_set_de_type()
>     error: buffer overflow 'ext4_type_by_mode' 15 <= 15
>
> It seems unlikely that we would hit this read overflow in real life, but
> it's also simple enough to make the array 16 bytes instead of 15.

FYI, I posted a series to fix this in 10 different filesystems, all have
the same hypothetical bug:
https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2
There were objections about using common code for filesystem
specific on-disk format, so my initial approach was dropped.

My manual code audit of ext4 concluded that this specific bug cannot
hit current ext4 code, which sanitizes the value of i_mode when reading
it from disk:
https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2

The only maintainer that followed up on fixing the array overflow was
Darrick and a fix was merged to xfs:
1fc4d33fed12 xfs: replace xfs_mode_to_ftype table with switch statement

So there are 6 more fix patches you can send.

Cheers,
Amir.

>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 9ebde0cd632e..fbaeb441bed3 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3074,7 +3074,7 @@ extern int ext4_handle_dirty_dirent_node(handle_t *handle,
>                                          struct inode *inode,
>                                          struct buffer_head *bh);
>  #define S_SHIFT 12
> -static const unsigned char ext4_type_by_mode[S_IFMT >> S_SHIFT] = {
> +static const unsigned char ext4_type_by_mode[(S_IFMT >> S_SHIFT) + 1] = {
>         [S_IFREG >> S_SHIFT]    = EXT4_FT_REG_FILE,
>         [S_IFDIR >> S_SHIFT]    = EXT4_FT_DIR,
>         [S_IFCHR >> S_SHIFT]    = EXT4_FT_CHRDEV,

I still think it makes more sense to use common DT_* macros here,
even if not using common conversion helpers, i.e.:
https://marc.info/?l=linux-fsdevel&m=148221950110233&w=2

If for nothing else, then for not redefining S_SHIFT 7 times
in the code:
fs/btrfs/inode.c:#define S_SHIFT 12
fs/exofs/dir.c:#define S_SHIFT 12
fs/ext2/dir.c:#define S_SHIFT 12
fs/ext4/ext4.h:#define S_SHIFT 12
fs/nilfs2/dir.c:#define S_SHIFT 12
fs/ocfs2/ocfs2_fs.h:#define S_SHIFT 12
include/linux/f2fs_fs.h:#define S_SHIFT 12



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux