[PATCH] xfs: eliminate the potential overflow risk in xfs_da_grow_inode_int

[PATCH] xfs: eliminate the potential overflow risk in xfs_da_grow_inode_int

[Stephen Zhang]

The problem lies in the for-loop of xfs_da_grow_inode_int:
======
for(){
        nmap = min(XFS_BMAP_MAX_NMAP, count);
        ...
        error = xfs_bmapi_write(...,&mapp[mapi], &nmap);//(..., $1, $2)
        ...
        mapi += nmap;
}
=====
where $1 stands for the start address of the array,
while $2 is used to indicate the size of the array.

The array $1 will advanced by $nmap in each iteration after
the allocation of extents.
But the size $2 still remains constant, which is determined by
min(XFS_BMAP_MAX_NMAP, count).

Hence there is a risk of overflow when the remained space in
the array is less than $2.
So variablize the array size $2 correspondingly in each iteration
to eliminate the risk.

Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
---
 fs/xfs/libxfs/xfs_da_btree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index e7201dc68f43..3ef8c04624cc 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -2192,7 +2192,7 @@ xfs_da_grow_inode_int(
 		 */
 		mapp = kmem_alloc(sizeof(*mapp) * count, 0);
 		for (b = *bno, mapi = 0; b < *bno + count; ) {
-			nmap = min(XFS_BMAP_MAX_NMAP, count);
+			nmap = min(XFS_BMAP_MAX_NMAP, count - mapi);
 			c = (int)(*bno + count - b);
 			error = xfs_bmapi_write(tp, dp, b, c,
 					xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA,
-- 
2.25.1

Re: [PATCH] xfs: eliminate the potential overflow risk in xfs_da_grow_inode_int

[Dave Chinner]

On Sat, Sep 10, 2022 at 10:38:39AM +0800, Stephen Zhang wrote:
> The problem lies in the for-loop of xfs_da_grow_inode_int:
> ======
> for(){
>         nmap = min(XFS_BMAP_MAX_NMAP, count);
>         ...
>         error = xfs_bmapi_write(...,&mapp[mapi], &nmap);//(..., $1, $2)
>         ...
>         mapi += nmap;
> }
> =====
> where $1 stands for the start address of the array,
> while $2 is used to indicate the size of the array.
> 
> The array $1 will advanced by $nmap in each iteration after
> the allocation of extents.
> But the size $2 still remains constant, which is determined by
> min(XFS_BMAP_MAX_NMAP, count).
> 
> Hence there is a risk of overflow when the remained space in
> the array is less than $2.
> So variablize the array size $2 correspondingly in each iteration
> to eliminate the risk.

Except that xfs_bmapi_write() won't overrun the array....

> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
>  fs/xfs/libxfs/xfs_da_btree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index e7201dc68f43..3ef8c04624cc 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -2192,7 +2192,7 @@ xfs_da_grow_inode_int(
>  		 */
>  		mapp = kmem_alloc(sizeof(*mapp) * count, 0);
>  		for (b = *bno, mapi = 0; b < *bno + count; ) {
> -			nmap = min(XFS_BMAP_MAX_NMAP, count);
> +			nmap = min(XFS_BMAP_MAX_NMAP, count - mapi);
>  			c = (int)(*bno + count - b);
>  			error = xfs_bmapi_write(tp, dp, b, c,
>  					xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA,


... because we've allocated a mapp array large enough for one extent
map per filesystem block.

The line:

	c = (int)(*bno + count - b);

calculates the maximum length of the extent remaining to map, and
hence the maximum number of blocks we might need to map.  We're
guaranteed that the array is large enough for all single block maps,
and xfs_bmapi_write() will never overrun the array because it doesn't
map extents beyond the length requested. IOWs, there isn't an array
overrun bug here even though we don't trim the requested number of
maps on the last call.

So the question remains: Why do we need *two* calculations that
calculate the remaining number of blocks to map here? i.e. surely
all we need is this:

-			nmap = min(XFS_BMAP_MAX_NMAP, count);
 			c = (int)(*bno + count - b);
+			nmap = min(XFS_BMAP_MAX_NMAP, c);

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com