linux-ext4 - Re: [PATCH] ext4: silence array overflow warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Jul 2017 14:00:21 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Ext4 <linux-ext4@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "Darrick J. Wong" <darrick.wong@...cle.com>
Subject: Re: [PATCH] ext4: silence array overflow warning

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@...cle.com> 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@...cle.com>
>
> 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