linux-ext4 - Re: [PATCH] ext4: avoid declaring fs inconsistent due to invalid file handles

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:   Mon, 17 Dec 2018 15:53:46 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        stable@...nel.org
Subject: Re: [PATCH] ext4: avoid declaring fs inconsistent due to invalid file
 handles


> On Dec 13, 2018, at 10:51 AM, Theodore Ts'o <tytso@....edu> wrote:
> 
> If we receive a file handle, either from NFS or open_by_handle_at(2),
> and it points at an inode which has not been initialized, and the file
> system has metadata checksums enabled, we shouldn't try to get the
> inode, discover the checksum is invalid, and then declare the file
> system as being inconsistent.
> 
> This can be reproduced by creating a test file system via "mke2fs -t
> ext4 -O metadata_csum /tmp/foo.img 8M", mounting it, cd'ing into that
> directory, and then running the following program.
> 
> #define _GNU_SOURCE
> #include <fcntl.h>
> 
> struct handle {
> 	struct file_handle fh;
> 	unsigned char fid[MAX_HANDLE_SZ];
> };
> 
> int main(int argc, char **argv)
> {
> 	struct handle h = {{8, 1 }, { 12, }};
> 
> 	open_by_handle_at(AT_FDCWD, &h.fh, O_RDONLY);
> 	return 0;
> }
> 
> Addresses-Google-Bug: #120690101
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> Cc: stable@...nel.org
> ---
> fs/ext4/ext4.h   | 12 +++++++++--
> fs/ext4/ialloc.c |  2 +-
> fs/ext4/inode.c  | 54 +++++++++++++++++++++++++++++++++---------------
> fs/ext4/ioctl.c  |  2 +-
> fs/ext4/namei.c  |  4 ++--
> fs/ext4/resize.c |  4 ++--
> fs/ext4/super.c  | 19 +++++------------
> fs/ext4/xattr.c  |  5 +++--
> 8 files changed, 61 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b4621277e259..20eaa66705cc 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2454,8 +2454,16 @@ int do_journal_get_write_access(handle_t *handle,
> #define FALL_BACK_TO_NONDELALLOC 1
> #define CONVERT_INLINE_DATA	 2
> 
> -extern struct inode *ext4_iget(struct super_block *, unsigned long);
> -extern struct inode *ext4_iget_normal(struct super_block *, unsigned long);
> +extern struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
> +				 int flags, const char *function,
> +				 unsigned int line);
> +
> +#define EXT4_IGET_NORMAL	1
> +#define EXT4_IGET_HANDLE	2

It would be better to make this:


 enum ext4_iget_flags {
        EXT4_IGET_RESERVED = 0x00,	/* just guessing, see further below */
	EXT4_IGET_NORMAL   = 0x01,
	EXT4_IGET_HANDLE   = 0x02,
 };

so that it is clear which "flags" are being passed to ext4_iget().  There are
hundreds of "flags" arguments to different functions that are sometimes hard
to separate when looking at code.  That also gives the compiler some chance to
warn if they are used incorrectly.

> +#define ext4_iget(sb, ino, flags) \
> +	__ext4_iget((sb), (ino), (flags), __func__, __LINE__)
> +
> extern int  ext4_write_inode(struct inode *, struct writeback_control *);
> extern int  ext4_setattr(struct dentry *, struct iattr *);
> extern int  ext4_getattr(const struct path *, struct kstat *, u32, unsigned int);
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 014f6a698cb7..06aa3b6c3f18 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1225,7 +1225,7 @@ struct inode *ext4_orphan_get(struct super_block *sb, unsigned long ino)
> 	if (!ext4_test_bit(bit, bitmap_bh->b_data))
> 		goto bad_orphan;
> 
> -	inode = ext4_iget(sb, ino);
> +	inode = ext4_iget(sb, ino, 0);

What does "0" mean?  It isn't in the list of EXT4_IGET_* flags.  I'm guessing it
means that access to reserved or otherwise invalid inodes is allowed?

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 22a9d8159720..07fc7a12508e 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4817,7 +4817,9 @@ static inline u64 ext4_inode_peek_iversion(const struct inode *inode)
> 		return inode_peek_iversion(inode);
> }
> 
> -struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> +struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
> +			  int flags, const char *function,

			   enum ext4_iget_flags flags, ...

> @@ -4831,6 +4833,18 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 	gid_t i_gid;
> 	projid_t i_projid;
> 
> +	if (((flags & EXT4_IGET_NORMAL) &&
> +	     (ino < EXT4_FIRST_INO(sb) && ino != EXT4_ROOT_INO)) ||
> +	    (ino < EXT4_ROOT_INO) ||

This check seems redundant with "ino < EXT4_FIRST_INO(sb)" on the previous line?

> +	    (ino > le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count))) {
> +		if (flags & EXT4_IGET_HANDLE)
> +			return ERR_PTR(-ESTALE);
> +		__ext4_error(sb, function, line,
> +			     "inode #%lu: comm %s: iget: illegal inode #",
> +			     ino, current->comm);
> +		return ERR_PTR(-EFSCORRUPTED);
> +	}


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)