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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e3da1bba-9740-6b6f-385a-1bdf25f056a9@huawei.com>
Date: Sat, 7 Dec 2024 12:17:33 +0800
From: Zhihao Cheng <chengzhihao1@...wei.com>
To: Dan Carpenter <dan.carpenter@...aro.org>,
	Michał Kępień <kernel@...pniu.pl>
CC: Miquel Raynal <miquel.raynal@...tlin.com>, Richard Weinberger
	<richard@....at>, Vignesh Raghavendra <vigneshr@...com>,
	<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH] mtdchar: fix integer overflow in read/write ioctls

在 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@...aro.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;
>   	}
>