WARN fibmap user in case of possible addr truncation
Subsystems (3)
cover WARN fibmap user in case of possible addr truncation
Currently an ioctl_fibmap() caller may get a truncated and wrong block map address if the actual mapped physical block address is > INT_MAX. If that is the case then lets:-
1. Add a warning,
2. Returns 0 in the block mapped address,
3. And also return -ERANGE error.
This is better than providing a wrong information to the user about the block mapping which in the worst case could cause a FS corruption (unknowingly by the caller since kernel didn't warn).
Currently these checks are in place only for the filesystems who uses iomap based bmap interface. So let's do this right thing for all the fibmap() callers by adding this logic in ioctl_fibmap() directly.
Patch-1 & Patch-2 commit msg may provide additional information.
Ritesh Harjani (2):
fibmap: Warn and return an error in case of block > INT_MAX
iomap: bmap: Remove the WARN and return the proper block address
fs/ioctl.c | 5 +++++ fs/iomap/fiemap.c | 5 +---- 2 files changed, 6 insertions(+), 4 deletions(-)
Signature · 1 line
2.21.0
PATCH 1/2 fibmap: Warn and return an error in case of block > INT_MAX
We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the bmap() returned block address is greater than INT_MAX (since user supplied integer pointer).
It's better to WARN all user of ioctl_fibmap() and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.
We fix this by returning an error code of -ERANGE and returning 0 as the block mapping address in case if it is > INT_MAX.
--- fs/ioctl.c | 5 +++++ 1 file changed, 5 insertions(+)
fs/ioctl.c +5 -0
@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) block = ur_block; error =bmap (inode, &block); +if (block >INT_MAX ) { + error = -ERANGE ; +WARN (1 ,"would truncate fibmap result\n" ); + } +if (error) ur_block =0 ;else
Signature · 1 line
2.21.0
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 4 lines · Ritesh Harjani
We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the
bmap()returned block address is greater thanINT_MAX(since user supplied integer pointer).
Quoted 8 lines
It's better to WARN all user of
ioctl_fibmap()and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.We fix this by returning an error code of
-ERANGEand returning 0 as the block mapping address in case if it is >INT_MAX.Signed-off-by Ritesh Harjani <[email protected]>
The patch looks good to me. You can add:
Honza
Quoted 23 lines
--- fs/ioctl.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ioctl.c +5
-0@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) block = ur_block; error =bmap (inode, &block); +if (block >INT_MAX ) { + error = -ERANGE ; +WARN (1 ,"would truncate fibmap result\n" ); + } +if (error) ur_block =0 ;else Signature · 1 line
2.21.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
In reply to Ritesh HarjaniQuoted 4 lines · Ritesh Harjani
We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the
bmap()returned block address is greater thanINT_MAX(since user supplied integer pointer).
Quoted 8 lines
It's better to WARN all user of
ioctl_fibmap()and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.We fix this by returning an error code of
-ERANGEand returning 0 as the block mapping address in case if it is >INT_MAX.Signed-off-by Ritesh Harjani <[email protected]>
Looks good,
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
In reply to Ritesh HarjaniQuoted 21 lines · Ritesh Harjani
We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the
bmap()returned block address is greater thanINT_MAX(since user supplied integer pointer).It's better to WARN all user of
ioctl_fibmap()and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.We fix this by returning an error code of
-ERANGEand returning 0 as the block mapping address in case if it is >INT_MAX.Signed-off-by Ritesh Harjani <[email protected]>--- fs/ioctl.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ioctl.c +0
-0
fs/ioctl.c static int ioctl_fibmap(struct file *filp, int __user *p) · line 78
Quoted 8 lines
fs/ioctl.c +5
-0@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) block = ur_block; error =bmap (inode, &block); +if (block >INT_MAX ) { + error = -ERANGE ; +WARN (1 ,"would truncate fibmap result\n" ); + } +
WARN() is only for kernel bugs. This case would be a userspace bug, not a kernel bug, right? If so, it should use pr_warn(), not WARN().
- Eric
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 25 lines · Eric Biggers
Quoted 24 lines · Ritesh Harjani
We better warn the fibmap user and not return a truncated and therefore an incorrect block map address if the
bmap()returned block address is greater thanINT_MAX(since user supplied integer pointer).It's better to WARN all user of
ioctl_fibmap()and return a proper error code rather than silently letting a FS corruption happen if the user tries to fiddle around with the returned block map address.We fix this by returning an error code of
-ERANGEand returning 0 as the block mapping address in case if it is >INT_MAX.Signed-off-by Ritesh Harjani <[email protected]>--- fs/ioctl.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ioctl.c +0
-0@@ -71,6 +71,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p) block = ur_block; error =bmap (inode, &block);
Quoted 8 lines
fs/ioctl.c static int ioctl_fibmap(struct file *filp, int __user *p) · line 78
Quoted 5 lines
fs/ioctl.c +5
-0+if (block >INT_MAX ) { + error = -ERANGE ; +WARN (1 ,"would truncate fibmap result\n" ); + } +
WARN()is only for kernel bugs. This case would be a userspace bug, not a kernel bug, right? If so, it should usepr_warn(), notWARN().
Ok, I see.
Let me replace WARN() with below pr_warn() line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).
pr_warn("fibmap: this would truncate fibmap result\n");
Quoted 3 lines
- Eric
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 6 lines · Ritesh Harjani
Ok, I see.
Let me replaceWARN()with belowpr_warn()line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).
pr_warn("fibmap: this would truncate fibmap result\n");
We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles. printk_ratelimited()?
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 10 lines · Matthew Wilcox
Quoted 6 lines · Ritesh Harjani
Ok, I see.
Let me replaceWARN()with belowpr_warn()line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).
pr_warn("fibmap: this would truncate fibmap result\n");We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles.
printk_ratelimited()?
Or WARN_ON_ONCE...
--D
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 7 lines · Darrick J. Wong
Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Ritesh Harjani
Ok, I see.
Let me replaceWARN()with belowpr_warn()line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
pr_warn("fibmap: this would truncate fibmap result\n");We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles.
printk_ratelimited()?Or
WARN_ON_ONCE...
So, Eric was mentioning WARN_** are mostly for kernel side of bugs. But this is mostly a API fault which affects user side and also to warn the user about the possible truncation in the block fibmap addr.
Also WARN_ON_ONCE, will be shown only once and won't be printed for every other file for which block addr > INT_MAX.
I think we could go with below. If ok, I could post this in v2.
pr_warn_ratelimited("fibmap: would truncate fibmap result\n");
-ritesh
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 24 lines · Ritesh Harjani
Quoted 7 lines · Darrick J. Wong
Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Ritesh Harjani
Ok, I see.
Let me replaceWARN()with belowpr_warn()line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).Quoted 6 lines
Quoted 4 lines
Quoted 1 line
pr_warn("fibmap: this would truncate fibmap result\n");We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles.
printk_ratelimited()?Or
WARN_ON_ONCE...So, Eric was mentioning WARN_** are mostly for kernel side of bugs. But this is mostly a API fault which affects user side and also to warn the user about the possible truncation in the block fibmap addr.
AlsoWARN_ON_ONCE, will be shown only once and won't be printed for every other file for which block addr >INT_MAX.
Quoted 3 lines
I think we could go with below. If ok, I could post this in v2.
pr_warn_ratelimited("fibmap: would truncate fibmap result\n");
Please include the process ID, the superblock ID and the task name that is triggering this warning. Otherwise the administrator will have no clue what is generating it and so won't be able to fix it...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 1/2] fibmap: Warn and return an error in case of block > INT_MAX
Quoted 25 lines · Dave Chinner
Quoted 24 lines · Ritesh Harjani
Quoted 7 lines · Darrick J. Wong
Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Ritesh Harjani
Ok, I see.
Let me replaceWARN()with belowpr_warn()line then. If no objections, then will send this in a v2 with both patches combined as Darrick suggested. - (with Reviewed-by tags of Jan & Christoph).Quoted 6 lines
Quoted 4 lines
Quoted 1 line
pr_warn("fibmap: this would truncate fibmap result\n");We generally don't like userspace to be able to trigger kernel messages on demand, so they can't swamp the logfiles.
printk_ratelimited()?Or
WARN_ON_ONCE...So, Eric was mentioning WARN_** are mostly for kernel side of bugs. But this is mostly a API fault which affects user side and also to warn the user about the possible truncation in the block fibmap addr.
AlsoWARN_ON_ONCE, will be shown only once and won't be printed for every other file for which block addr >INT_MAX.
Quoted 8 lines
Quoted 3 lines
I think we could go with below. If ok, I could post this in v2.
pr_warn_ratelimited("fibmap: would truncate fibmap result\n");Please include the process ID, the superblock ID and the task name that is triggering this warning. Otherwise the administrator will have no clue what is generating it and so won't be able to fix it...
Thanks for the suggestion. I will make it like below then. Will send a v2 soon.
+ pr_warn_ratelimited("[%s/%d] FS (%s): would truncate fibmap result\n",
+ current->comm, task_pid_nr(current),
+ sb->s_id);
+
PATCH 2/2 iomap: bmap: Remove the WARN and return the proper block address
In reply to Ritesh Harjaniiomap_bmap() could be called from either of these two paths. Either when a user is calling an ioctl_fibmap() interface to get the block mapping address or by some filesystem via use of bmap() internal kernel API.bmap() kernel API is well equipped with handling of u64 addresses.
WARN condition in iomap_bmap_actor() was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr > INT_MAX.
So we can now remove this logic from here.
--- fs/iomap/fiemap.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
fs/iomap/fiemap.c +1 -4
@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,if (iomap->type ==IOMAP_MAPPED ) { addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; -if (addr >INT_MAX ) -WARN (1 ,"would truncate bmap result\n" ); -else - *bno = addr; + *bno = addr; }return 0 ; }
Signature · 1 line
2.21.0
Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address
Quoted 6 lines · Ritesh Harjani
iomap_bmap()could be called from either of these two paths. Either when a user is calling anioctl_fibmap()interface to get the block mapping address or by some filesystem via use ofbmap()internal kernel API.bmap()kernel API is well equipped with handling of u64 addresses.
Quoted 7 lines
WARN condition in
iomap_bmap_actor()was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr >INT_MAX.
So we can now remove this logic from here.Signed-off-by Ritesh Harjani <[email protected]>
Yes, I agree it's better to hadle the overflow in the ioctl than in the iomap actor. The patch looks good to me. You can add:
Honza
Quoted 23 lines
--- fs/iomap/fiemap.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)fs/iomap/fiemap.c +1
-4@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,if (iomap->type ==IOMAP_MAPPED ) { addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; -if (addr >INT_MAX ) -WARN (1 ,"would truncate bmap result\n" ); -else - *bno = addr; + *bno = addr; }return 0 ; }Signature · 1 line
2.21.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address
In reply to Ritesh HarjaniLooks good,
Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address
In reply to Ritesh HarjaniQuoted 22 lines · Ritesh Harjani
iomap_bmap()could be called from either of these two paths. Either when a user is calling anioctl_fibmap()interface to get the block mapping address or by some filesystem via use ofbmap()internal kernel API.bmap()kernel API is well equipped with handling of u64 addresses.WARN condition in
iomap_bmap_actor()was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr >INT_MAX.
So we can now remove this logic from here.Signed-off-by Ritesh Harjani <[email protected]>--- fs/iomap/fiemap.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)fs/iomap/fiemap.c +0
-0
fs/iomap/fiemap.c iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, · line 121
Quoted 5 lines
fs/iomap/fiemap.c +0
-2@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,if (iomap->type ==IOMAP_MAPPED ) { addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; -if (addr >INT_MAX ) -WARN (1 ,"would truncate bmap result\n" );
Frankly I would've combined these two patches to make it more obvious that we're hoisting a FIBMAP constraint check from iomap into the ioctl handler.
--D
Quoted 9 lines
fs/iomap/fiemap.c +1
-2-else - *bno = addr; + *bno = addr; }return 0 ; }Signature · 1 line
2.21.0
Re: [PATCH 2/2] iomap: bmap: Remove the WARN and return the proper block address
Quoted 24 lines · Darrick J. Wong
Quoted 23 lines · Ritesh Harjani
iomap_bmap()could be called from either of these two paths. Either when a user is calling anioctl_fibmap()interface to get the block mapping address or by some filesystem via use ofbmap()internal kernel API.bmap()kernel API is well equipped with handling of u64 addresses.WARN condition in
iomap_bmap_actor()was mainly added to warn all the fibmap users. But now that in previous patch we have directly added this WARN condition for all fibmap users and also made sure to return 0 as block map address in case if addr >INT_MAX.
So we can now remove this logic from here.Signed-off-by Ritesh Harjani <[email protected]>--- fs/iomap/fiemap.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)fs/iomap/fiemap.c +0
-0@@ -117,10 +117,7 @@ iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length,
Quoted 8 lines
fs/iomap/fiemap.c iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, · line 121
Quoted 4 lines
fs/iomap/fiemap.c +0
-2if (iomap->type ==IOMAP_MAPPED ) { addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; -if (addr >INT_MAX ) -WARN (1 ,"would truncate bmap result\n" );Frankly I would've combined these two patches to make it more obvious that we're hoisting a FIBMAP constraint check from iomap into the ioctl handler.
Sure, let me combine the two in v2.
Thanks!!
-ritesh
Quoted 12 lines
--D
Quoted 9 lines
fs/iomap/fiemap.c +1
-2-else - *bno = addr; + *bno = addr; }return 0 ; }Signature · 1 line
2.21.0