Re: [PATCH] mtdchar: fix integer overflow in read/write ioctls

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 2024/12/7 4:26, Dan Carpenter 写道:
> The "req.start" and "req.len" variables are u64 values that come from the
> user at the start of the function.  We mask away the high 32 bits of
> "req.len" so that's capped at U32_MAX but the "req.start" variable can go
> up to U64_MAX.
> 
> Use check_add_overflow() to fix this bug.
> 
> Fixes: 6420ac0af95d ("mtdchar: prevent unbounded allocation in MEMWRITE ioctl")

Hi, Dan. Why this fix tag? I think the adding result('req.start' and 
'req.len') could be overflow too before this commit.

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/mtd/mtdchar.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 8dc4f5c493fc..335c702633ff 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -599,6 +599,7 @@ mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>   	uint8_t *datbuf = NULL, *oobbuf = NULL;
>   	size_t datbuf_len, oobbuf_len;
>   	int ret = 0;
> +	u64 end;
>   
>   	if (copy_from_user(&req, argp, sizeof(req)))
>   		return -EFAULT;
> @@ -618,7 +619,7 @@ mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>   	req.len &= 0xffffffff;
>   	req.ooblen &= 0xffffffff;
>   
> -	if (req.start + req.len > mtd->size)
> +	if (check_add_overflow(req.start, req.len, &end) || end > mtd->size)
>   		return -EINVAL;
>   
>   	datbuf_len = min_t(size_t, req.len, mtd->erasesize);
> @@ -698,6 +699,7 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>   	size_t datbuf_len, oobbuf_len;
>   	size_t orig_len, orig_ooblen;
>   	int ret = 0;
> +	u64 end;
>   
>   	if (copy_from_user(&req, argp, sizeof(req)))
>   		return -EFAULT;
> @@ -724,7 +726,7 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>   	req.len &= 0xffffffff;
>   	req.ooblen &= 0xffffffff;
>   
> -	if (req.start + req.len > mtd->size) {
> +	if (check_add_overflow(req.start, req.len, &end) || end > mtd->size) {
>   		ret = -EINVAL;
>   		goto out;
>   	}
>