* [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 5:53 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 5:53 UTC (permalink / raw) To: Ben Myers; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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 || + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { xfs_warn(ip->i_mount, "corrupt inode %Lu (bad size %Ld for local inode).", (unsigned long long) ip->i_ino, ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 5:53 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 5:53 UTC (permalink / raw) To: Ben Myers; +Cc: Alex Elder, xfs, linux-kernel, kernel-janitors 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@oracle.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 || + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { xfs_warn(ip->i_mount, "corrupt inode %Lu (bad size %Ld for local inode).", (unsigned long long) ip->i_ino, ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 5:53 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 5:53 UTC (permalink / raw) To: Ben Myers; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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 || + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { xfs_warn(ip->i_mount, "corrupt inode %Lu (bad size %Ld for local inode).", (unsigned long long) ip->i_ino, _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-15 5:53 ` Dan Carpenter (?) @ 2013-08-15 10:10 ` Jeff Liu -1 siblings, 0 replies; 25+ messages in thread From: Jeff Liu @ 2013-08-15 10:10 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ben Myers, Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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. > + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { > xfs_warn(ip->i_mount, > "corrupt inode %Lu (bad size %Ld for local inode).", > (unsigned long long) ip->i_ino, > Thanks, -Jeff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 10:10 ` Jeff Liu 0 siblings, 0 replies; 25+ messages in thread From: Jeff Liu @ 2013-08-15 10:10 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ben Myers, Alex Elder, xfs, linux-kernel, kernel-janitors 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@oracle.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. > + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { > xfs_warn(ip->i_mount, > "corrupt inode %Lu (bad size %Ld for local inode).", > (unsigned long long) ip->i_ino, > Thanks, -Jeff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 10:10 ` Jeff Liu 0 siblings, 0 replies; 25+ messages in thread From: Jeff Liu @ 2013-08-15 10:10 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ben Myers, Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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. > + di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) { > xfs_warn(ip->i_mount, > "corrupt inode %Lu (bad size %Ld for local inode).", > (unsigned long long) ip->i_ino, > Thanks, -Jeff _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-15 10:10 ` Jeff Liu (?) @ 2013-08-15 14:37 ` Ben Myers -1 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-15 14:37 UTC (permalink / raw) To: Dan Carpenter, Jeff Liu; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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@sgi.com> Regards, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 14:37 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-15 14:37 UTC (permalink / raw) To: Dan Carpenter, Jeff Liu; +Cc: Alex Elder, xfs, linux-kernel, kernel-janitors 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@oracle.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@sgi.com> Regards, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 14:37 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-15 14:37 UTC (permalink / raw) To: Dan Carpenter, Jeff Liu; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs 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@oracle.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@sgi.com> Regards, Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-15 14:37 ` Ben Myers (?) @ 2013-08-15 15:47 ` Dan Carpenter -1 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 15:47 UTC (permalink / raw) To: Ben Myers; +Cc: Jeff Liu, Alex Elder, kernel-janitors, linux-kernel, xfs On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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? > These are static checker things... It's too false positive prone to push on the real world yet. regards, dan carpenter ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 15:47 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 15:47 UTC (permalink / raw) To: Ben Myers; +Cc: Jeff Liu, Alex Elder, xfs, linux-kernel, kernel-janitors On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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? > These are static checker things... It's too false positive prone to push on the real world yet. regards, dan carpenter ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 15:47 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-15 15:47 UTC (permalink / raw) To: Ben Myers; +Cc: Jeff Liu, Alex Elder, kernel-janitors, linux-kernel, xfs On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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? > These are static checker things... It's too false positive prone to push on the real world yet. regards, dan carpenter _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-15 14:37 ` Ben Myers (?) @ 2013-08-15 22:26 ` Dave Chinner -1 siblings, 0 replies; 25+ messages in thread From: Dave Chinner @ 2013-08-15 22:26 UTC (permalink / raw) To: Ben Myers Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu, Dan Carpenter On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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. There are lots of places where we only check di_size to be greater than some value, and don't check for it being less than zero. Hence I think that a better solution might be to di_size unsigned as that will catch "negative" sizes for all types of situations. We've got the same problem in the userspace code as well and so treating the size as unsigned will stop such validation problems everywhere.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 22:26 ` Dave Chinner 0 siblings, 0 replies; 25+ messages in thread From: Dave Chinner @ 2013-08-15 22:26 UTC (permalink / raw) To: Ben Myers Cc: Dan Carpenter, Jeff Liu, Alex Elder, kernel-janitors, linux-kernel, xfs On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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. There are lots of places where we only check di_size to be greater than some value, and don't check for it being less than zero. Hence I think that a better solution might be to di_size unsigned as that will catch "negative" sizes for all types of situations. We've got the same problem in the userspace code as well and so treating the size as unsigned will stop such validation problems everywhere.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-15 22:26 ` Dave Chinner 0 siblings, 0 replies; 25+ messages in thread From: Dave Chinner @ 2013-08-15 22:26 UTC (permalink / raw) To: Ben Myers Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu, Dan Carpenter On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > 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@oracle.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. There are lots of places where we only check di_size to be greater than some value, and don't check for it being less than zero. Hence I think that a better solution might be to di_size unsigned as that will catch "negative" sizes for all types of situations. We've got the same problem in the userspace code as well and so treating the size as unsigned will stop such validation problems everywhere.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-15 22:26 ` Dave Chinner (?) @ 2013-08-23 17:36 ` Ben Myers -1 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-23 17:36 UTC (permalink / raw) To: Dan Carpenter; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu Dan, On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > 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@oracle.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. > > There are lots of places where we only check di_size to be greater > than some value, and don't check for it being less than zero. Hence > I think that a better solution might be to di_size unsigned as that > will catch "negative" sizes for all types of situations. What do you say to making di_size unsigned? Any interest? Thanks, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-23 17:36 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-23 17:36 UTC (permalink / raw) To: Dan Carpenter Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu, Dave Chinner Dan, On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > 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@oracle.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. > > There are lots of places where we only check di_size to be greater > than some value, and don't check for it being less than zero. Hence > I think that a better solution might be to di_size unsigned as that > will catch "negative" sizes for all types of situations. What do you say to making di_size unsigned? Any interest? Thanks, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-23 17:36 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-23 17:36 UTC (permalink / raw) To: Dan Carpenter; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu Dan, On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > 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@oracle.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. > > There are lots of places where we only check di_size to be greater > than some value, and don't check for it being less than zero. Hence > I think that a better solution might be to di_size unsigned as that > will catch "negative" sizes for all types of situations. What do you say to making di_size unsigned? Any interest? Thanks, Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-23 17:36 ` Ben Myers (?) @ 2013-08-26 14:37 ` Dan Carpenter -1 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-26 14:37 UTC (permalink / raw) To: Ben Myers; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > Dan, > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > 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@oracle.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. > > > > There are lots of places where we only check di_size to be greater > > than some value, and don't check for it being less than zero. Hence > > I think that a better solution might be to di_size unsigned as that > > will catch "negative" sizes for all types of situations. > > What do you say to making di_size unsigned? Any interest? > I'm not the right person to change "lots of places". Some of these are probably subtle. Just give me the reported-by and I'm happy. regards, dan carpenter ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-26 14:37 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-26 14:37 UTC (permalink / raw) To: Ben Myers Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu, Dave Chinner On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > Dan, > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > 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@oracle.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. > > > > There are lots of places where we only check di_size to be greater > > than some value, and don't check for it being less than zero. Hence > > I think that a better solution might be to di_size unsigned as that > > will catch "negative" sizes for all types of situations. > > What do you say to making di_size unsigned? Any interest? > I'm not the right person to change "lots of places". Some of these are probably subtle. Just give me the reported-by and I'm happy. regards, dan carpenter ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-26 14:37 ` Dan Carpenter 0 siblings, 0 replies; 25+ messages in thread From: Dan Carpenter @ 2013-08-26 14:37 UTC (permalink / raw) To: Ben Myers; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > Dan, > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > 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@oracle.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. > > > > There are lots of places where we only check di_size to be greater > > than some value, and don't check for it being less than zero. Hence > > I think that a better solution might be to di_size unsigned as that > > will catch "negative" sizes for all types of situations. > > What do you say to making di_size unsigned? Any interest? > I'm not the right person to change "lots of places". Some of these are probably subtle. Just give me the reported-by and I'm happy. regards, dan carpenter _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-26 14:37 ` Dan Carpenter (?) @ 2013-08-26 16:12 ` Ben Myers -1 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-26 16:12 UTC (permalink / raw) To: Dan Carpenter; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu Hey Dan, On Mon, Aug 26, 2013 at 05:37:15PM +0300, Dan Carpenter wrote: > On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > > Dan, > > > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > > 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@oracle.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. > > > > > > There are lots of places where we only check di_size to be greater > > > than some value, and don't check for it being less than zero. Hence > > > I think that a better solution might be to di_size unsigned as that > > > will catch "negative" sizes for all types of situations. > > > > What do you say to making di_size unsigned? Any interest? > > > > I'm not the right person to change "lots of places". Some of these > are probably subtle. Just give me the reported-by and I'm happy. I'll apply this for now, and we'll see if someone is interested enough to pick up the rest. Thanks, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-26 16:12 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-26 16:12 UTC (permalink / raw) To: Dan Carpenter Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu, Dave Chinner Hey Dan, On Mon, Aug 26, 2013 at 05:37:15PM +0300, Dan Carpenter wrote: > On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > > Dan, > > > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > > 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@oracle.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. > > > > > > There are lots of places where we only check di_size to be greater > > > than some value, and don't check for it being less than zero. Hence > > > I think that a better solution might be to di_size unsigned as that > > > will catch "negative" sizes for all types of situations. > > > > What do you say to making di_size unsigned? Any interest? > > > > I'm not the right person to change "lots of places". Some of these > are probably subtle. Just give me the reported-by and I'm happy. I'll apply this for now, and we'll see if someone is interested enough to pick up the rest. Thanks, Ben ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() @ 2013-08-26 16:12 ` Ben Myers 0 siblings, 0 replies; 25+ messages in thread From: Ben Myers @ 2013-08-26 16:12 UTC (permalink / raw) To: Dan Carpenter; +Cc: Alex Elder, kernel-janitors, linux-kernel, xfs, Jeff Liu Hey Dan, On Mon, Aug 26, 2013 at 05:37:15PM +0300, Dan Carpenter wrote: > On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: > > Dan, > > > > On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: > > > On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: > > > > 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@oracle.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. > > > > > > There are lots of places where we only check di_size to be greater > > > than some value, and don't check for it being less than zero. Hence > > > I think that a better solution might be to di_size unsigned as that > > > will catch "negative" sizes for all types of situations. > > > > What do you say to making di_size unsigned? Any interest? > > > > I'm not the right person to change "lots of places". Some of these > are probably subtle. Just give me the reported-by and I'm happy. I'll apply this for now, and we'll see if someone is interested enough to pick up the rest. Thanks, Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] xfs: check for underflow in xfs_iformat_fork() 2013-08-26 16:12 ` Ben Myers (?) (?) @ 2013-08-28 9:16 ` Jeff Liu -1 siblings, 0 replies; 25+ messages in thread From: Jeff Liu @ 2013-08-28 9:16 UTC (permalink / raw) To: Ben Myers; +Cc: Dan Carpenter, xfs@oss.sgi.com On 08/27/2013 12:12 AM, Ben Myers wrote: > Hey Dan, > > On Mon, Aug 26, 2013 at 05:37:15PM +0300, Dan Carpenter wrote: >> On Fri, Aug 23, 2013 at 12:36:13PM -0500, Ben Myers wrote: >>> Dan, >>> >>> On Fri, Aug 16, 2013 at 08:26:50AM +1000, Dave Chinner wrote: >>>> On Thu, Aug 15, 2013 at 09:37:06AM -0500, Ben Myers wrote: >>>>> 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@oracle.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. >>>> >>>> There are lots of places where we only check di_size to be greater >>>> than some value, and don't check for it being less than zero. Hence >>>> I think that a better solution might be to di_size unsigned as that >>>> will catch "negative" sizes for all types of situations. >>> >>> What do you say to making di_size unsigned? Any interest? >>> >> >> I'm not the right person to change "lots of places". Some of these >> are probably subtle. Just give me the reported-by and I'm happy. > > I'll apply this for now, and we'll see if someone is interested enough to pick > up the rest. Hi Ben, I just back from a longer vacation, will take care of the rest. Thanks, -Jeff _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-08-28 9:16 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-15 5:53 [patch] xfs: check for underflow in xfs_iformat_fork() Dan Carpenter 2013-08-15 5:53 ` Dan Carpenter 2013-08-15 5:53 ` Dan Carpenter 2013-08-15 10:10 ` Jeff Liu 2013-08-15 10:10 ` Jeff Liu 2013-08-15 10:10 ` Jeff Liu 2013-08-15 14:37 ` Ben Myers 2013-08-15 14:37 ` Ben Myers 2013-08-15 14:37 ` Ben Myers 2013-08-15 15:47 ` Dan Carpenter 2013-08-15 15:47 ` Dan Carpenter 2013-08-15 15:47 ` Dan Carpenter 2013-08-15 22:26 ` Dave Chinner 2013-08-15 22:26 ` Dave Chinner 2013-08-15 22:26 ` Dave Chinner 2013-08-23 17:36 ` Ben Myers 2013-08-23 17:36 ` Ben Myers 2013-08-23 17:36 ` Ben Myers 2013-08-26 14:37 ` Dan Carpenter 2013-08-26 14:37 ` Dan Carpenter 2013-08-26 14:37 ` Dan Carpenter 2013-08-26 16:12 ` Ben Myers 2013-08-26 16:12 ` Ben Myers 2013-08-26 16:12 ` Ben Myers 2013-08-28 9:16 ` Jeff Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.