Re: [PATCH v2] jfs: fix slab-out-of-bounds read in ea_get()

[Dave Kleikamp <dave.kleikamp@oracle.com>]

On 2/13/25 3:05PM, Qasim Ijaz wrote:
> During the "size_check" label in ea_get(), the code checks if the extended
> attribute list (xattr) size matches ea_size. If not, it logs
> "ea_get: invalid extended attribute" and calls print_hex_dump().
> 
> Here, EALIST_SIZE(ea_buf->xattr) returns 4110417968, which exceeds
> INT_MAX (2,147,483,647). Then ea_size is clamped:
> 
> 	int size = clamp_t(int, ea_size, 0, EALIST_SIZE(ea_buf->xattr));
> 
> Although clamp_t aims to bound ea_size between 0 and 4110417968, the upper
> limit is treated as an int, causing an overflow above 2^31 - 1. This leads
> "size" to wrap around and become negative (-184549328).
> 
> The "size" is then passed to print_hex_dump() (called "len" in
> print_hex_dump()), it is passed as type size_t (an unsigned
> type), this is then stored inside a variable called
> "int remaining", which is then assigned to "int linelen" which
> is then passed to hex_dump_to_buffer(). In print_hex_dump()
> the for loop, iterates through 0 to len-1, where len is
> 18446744073525002176, calling hex_dump_to_buffer()
> on each iteration:
> 
> 	for (i = 0; i < len; i += rowsize) {
> 		linelen = min(remaining, rowsize);
> 		remaining -= rowsize;
> 
> 		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> 				   linebuf, sizeof(linebuf), ascii);
> 	
> 		...
> 	}
> 	
> The expected stopping condition (i < len) is effectively broken
> since len is corrupted and very large. This eventually leads to
> the "ptr+i" being passed to hex_dump_to_buffer() to get closer
> to the end of the actual bounds of "ptr", eventually an out of
> bounds access is done in hex_dump_to_buffer() in the following
> for loop:
> 
> 	for (j = 0; j < len; j++) {
> 			if (linebuflen < lx + 2)
> 				goto overflow2;
> 			ch = ptr[j];
> 		...
> 	}
> 
> To fix this we should validate "EALIST_SIZE(ea_buf->xattr)"
> before it is utilised.

Agreed. The patch looks good. I'll add it to jfs-next

Shaggy

> 
> Reported-by: syzbot <syzbot+4e6e7e4279d046613bc5@syzkaller.appspotmail.com>
> Tested-by: syzbot <syzbot+4e6e7e4279d046613bc5@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=4e6e7e4279d046613bc5
> Fixes: d9f9d96136cb ("jfs: xattr: check invalid xattr size more strictly")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
> ---
>   v2:
> - Added Cc stable tag
> 
>   fs/jfs/xattr.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
> index 24afbae87225..7575c51cce9b 100644
> --- a/fs/jfs/xattr.c
> +++ b/fs/jfs/xattr.c
> @@ -559,11 +555,16 @@ static int ea_get(struct inode *inode, struct ea_buffer *ea_buf, int min_size)
>   
>         size_check:
>   	if (EALIST_SIZE(ea_buf->xattr) != ea_size) {
> -		int size = clamp_t(int, ea_size, 0, EALIST_SIZE(ea_buf->xattr));
> -
> -		printk(KERN_ERR "ea_get: invalid extended attribute\n");
> -		print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1,
> -				     ea_buf->xattr, size, 1);
> +		if (unlikely(EALIST_SIZE(ea_buf->xattr) > INT_MAX)) {
> +			printk(KERN_ERR "ea_get: extended attribute size too large: %u > INT_MAX\n",
> +			       EALIST_SIZE(ea_buf->xattr));
> +		} else {
> +			int size = clamp_t(int, ea_size, 0, EALIST_SIZE(ea_buf->xattr));
> +
> +			printk(KERN_ERR "ea_get: invalid extended attribute\n");
> +			print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1,
> +				       ea_buf->xattr, size, 1);
> +		}
>   		ea_release(inode, ea_buf);
>   		rc = -EIO;
>   		goto clean_up;