xfs: check for underflow in xfs_iformat_fork()

PATCH In review Dan Carpenter 4 participants 9 messages Aug 15, 2013 – Aug 28, 2013

Subsystems (2)

Kernel janitors XFS filesystem

lore

Share & tools

lore

Dan Carpenter

author

2013-08-15 05:53 UTC #0 lore raw

PATCH 1/1 xfs: check for underflow in xfs_iformat_fork()

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 <[email protected]>

fs/xfs/xfs_inode_fork.c +2 -1

index 123971b..849fc70 100644
@@ -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,

Jeff Liu

2013-08-15 10:10 UTC #1 lore raw

Quoted 11 lines · Dan Carpenter

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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644

fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170

Quoted 5 lines

fs/xfs/xfs_inode_fork.c +1 -1

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

fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 174

Quoted 5 lines

fs/xfs/xfs_inode_fork.c +1 -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,

Thanks,
-Jeff

Ben Myers

Reviewed-by

2013-08-15 14:37 UTC #2 lore raw

Hey Dan & Jeff,

Quoted 15 lines · Jeff Liu

Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 

Quoted 7 lines

fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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 <[email protected]>

Regards,
Ben

Dan Carpenter

author

2013-08-15 15:47 UTC #3 lore raw

Quoted 26 lines · Ben Myers

Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.

Quoted 8 lines

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

Dave Chinner

2013-08-15 22:26 UTC #4 lore raw

In reply to Ben Myers

Quoted 26 lines · Ben Myers

Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.

Quoted 5 lines

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.

Signature · 2 lines

Dave Chinner
[email protected]

Ben Myers

2013-08-23 17:36 UTC #5 lore raw

Dan,

Quoted 33 lines · Dave Chinner

Quoted 26 lines · Ben Myers
Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.
Quoted 5 lines
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.

Quoted 4 lines

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

Dan Carpenter

author

2013-08-26 14:37 UTC #6 lore raw

Quoted 36 lines · Ben Myers

Dan,
Quoted 33 lines · Dave Chinner
Quoted 31 lines · Ben Myers
Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.

Quoted 7 lines

Quoted 4 lines
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

Ben Myers

2013-08-26 16:12 UTC #7 lore raw

Hey Dan,

Quoted 42 lines · Dan Carpenter

Quoted 41 lines · Ben Myers
Dan,
Quoted 37 lines · Dave Chinner
Quoted 26 lines · Ben Myers
Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.
Quoted 5 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Jeff Liu

2013-08-28 09:16 UTC #8 lore raw

Quoted 45 lines · Ben Myers

Hey Dan,
Quoted 42 lines · Dan Carpenter
Quoted 41 lines · Ben Myers
Dan,
Quoted 37 lines · Dave Chinner
Quoted 26 lines · Ben Myers
Hey Dan & Jeff,
Quoted 15 lines · Jeff Liu
Quoted 13 lines · Dan Carpenter
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 <[email protected]>
fs/xfs/xfs_inode_fork.c +0 -0
index 123971b..849fc70 100644
@@ -167,7 +167,8 @@ xfs_iformat_fork(
 			}
 
Quoted 7 lines
fs/xfs/xfs_inode_fork.c xfs_iformat_fork( · line 170
Quoted 3 lines
fs/xfs/xfs_inode_fork.c +1 -1
 			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.
Quoted 5 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs