linux-kernel - Re: [patch] xfs: check for underflow in xfs_iformat

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]

Message-ID: <20130815143706.GI7153@sgi.com>
Date:	Thu, 15 Aug 2013 09:37:06 -0500
From:	Ben Myers <bpm@....com>
To:	Dan Carpenter <dan.carpenter@...cle.com>,
	Jeff Liu <jeff.liu@...cle.com>
Cc:	Alex Elder <elder@...nel.org>, xfs@....sgi.com,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [patch] xfs: check for underflow in xfs_iformat_fork()

Hey Dan & Jeff,

On Thu, Aug 15, 2013 at 06:10:43PM +0800, Jeff Liu wrote:
> On 08/15/2013 01:53 PM, Dan Carpenter wrote:
> 
> > The "di_size" variable comes from the disk and it's a signed 64 bit.
> > We check the upper limit but we should check for negative numbers as
> > well.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> > 
> > diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
> > index 123971b..849fc70 100644
> > --- a/fs/xfs/xfs_inode_fork.c
> > +++ b/fs/xfs/xfs_inode_fork.c
> > @@ -167,7 +167,8 @@ xfs_iformat_fork(
> >  			}
> >  
> >  			di_size = be64_to_cpu(dip->di_size);
> > -			if (unlikely(di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
> > +			if (unlikely(di_size < 0 ||
> 
> But the di_size is initialized to ZERO while allocating a new inode on disk.
> I wonder if that is better to ASSERT in this case because the current check
> is used to make sure that the item is inlined, or we don't need it at all.

Hmm.  Dan's additional check looks good to me.  In this case I'd say the forced
shutdown is more appropriate than an assert, because here we're reading the
inode from disk, as opposed to looking at a structure that is already incore
which we think we've initialized.  We want to handle unexpected inputs from
disk without crashing even if we are CONFIG_XFS_DEBUG.

How did you come across this one?

Reviewed-by: Ben Myers <bpm@....com>

Regards,
Ben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/