linux-ext4 - Re: [PATCH 3/3] ext2: Strengthen xattr block checks

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] [day] [month] [year] [list]

Message-ID: <20190516082944.GB13274@quack2.suse.cz>
Date:   Thu, 16 May 2019 10:29:44 +0200
From:   Jan Kara <jack@...e.cz>
To:     "cgxu519@...o.com.cn" <cgxu519@...o.com.cn>
Cc:     Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 3/3] ext2: Strengthen xattr block checks

On Thu 16-05-19 09:16:06, cgxu519@...o.com.cn wrote:
> On Wed, 2019-05-15 at 16:01 +0200, Jan Kara wrote:
> > Check every entry in xattr block for validity in ext2_xattr_set() to
> > detect on disk corruption early. Also since e_value_block field in xattr
> > entry is never != 0 in a valid filesystem, just remove checks for it
> > once we have established entries are valid.
> > 
> > Signed-off-by: Jan Kara <jack@...e.cz>
> 
> Could we do the entry check in the loop of get/list operation too?

Yes, makes sense. Will add for v2. Thanks for review.

								Honza

> 
> Thanks,
> Chengguang
> 
> > ---
> >  fs/ext2/xattr.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > index 26a049ca89fb..04a4148d04b3 100644
> > --- a/fs/ext2/xattr.c
> > +++ b/fs/ext2/xattr.c
> > @@ -442,7 +442,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			struct ext2_xattr_entry *next = EXT2_XATTR_NEXT(last);
> >  			if ((char *)next >= end)
> >  				goto bad_block;
> > -			if (!last->e_value_block && last->e_value_size) {
> > +			if (!ext2_xattr_entry_valid(last, sb->s_blocksize))
> > +				goto bad_block;
> > +			if (last->e_value_size) {
> >  				size_t offs = le16_to_cpu(last->e_value_offs);
> >  				if (offs < min_offs)
> >  					min_offs = offs;
> > @@ -482,12 +484,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  		error = -EEXIST;
> >  		if (flags & XATTR_CREATE)
> >  			goto cleanup;
> > -		if (!here->e_value_block && here->e_value_size) {
> > -			if (!ext2_xattr_entry_valid(here, sb->s_blocksize))
> > -				goto bad_block;
> > -			free += EXT2_XATTR_SIZE(
> > -					le32_to_cpu(here->e_value_size));
> > -		}
> > +		free += EXT2_XATTR_SIZE(le32_to_cpu(here->e_value_size));
> >  		free += EXT2_XATTR_LEN(name_len);
> >  	}
> >  	error = -ENOSPC;
> > @@ -552,7 +549,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  		here->e_name_len = name_len;
> >  		memcpy(here->e_name, name, name_len);
> >  	} else {
> > -		if (!here->e_value_block && here->e_value_size) {
> > +		if (here->e_value_size) {
> >  			char *first_val = (char *)header + min_offs;
> >  			size_t offs = le16_to_cpu(here->e_value_offs);
> >  			char *val = (char *)header + offs;
> > @@ -579,7 +576,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const
> > char *name,
> >  			last = ENTRY(header+1);
> >  			while (!IS_LAST_ENTRY(last)) {
> >  				size_t o = le16_to_cpu(last->e_value_offs);
> > -				if (!last->e_value_block && o < offs)
> > +				if (o < offs)
> >  					last->e_value_offs =
> >  						cpu_to_le16(o + size);
> >  				last = EXT2_XATTR_NEXT(last);
> 
> 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR