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")
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;
 	}
-- 
2.45.2

在 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;
>   	}
> 

On Sat, Dec 07, 2024 at 12:17:33PM +0800, Zhihao Cheng wrote:
> 在 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.
> 

I've looked at this again, and I still don't see the bug before the
commit.  Secondly, commit a1eda864c04c ("mtdchar: prevent integer
overflow in a safety check") is missing a Fixes tag but the message says
that it's this commit which introduced the bug.

Which commit should get the fixes tag?

I should have added a CC to the stable tree though.  I did that correctly
in an earlier draft of this patch but I messed up in this version. :/

regards,
dan carpenter

在 2024/12/8 1:05, Dan Carpenter 写道:
> On Sat, Dec 07, 2024 at 12:17:33PM +0800, Zhihao Cheng wrote:
>> 在 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.
>>
> 
> I've looked at this again, and I still don't see the bug before the
> commit.  Secondly, commit a1eda864c04c ("mtdchar: prevent integer
> overflow in a safety check") is missing a Fixes tag but the message says
> that it's this commit which introduced the bug.

Ah, I see. There is not an addition operation for 'req.start' and 
'req.len' until commit 6420ac0af95d("mtdchar: prevent unbounded 
allocation in MEMWRITE ioctl") and 095bb6e44eb1("mtdchar: add MEMREAD 
ioctl"), so I guess the there should be two fix tags?
> 
> Which commit should get the fixes tag?
> 
> I should have added a CC to the stable tree though.  I did that correctly
> in an earlier draft of this patch but I messed up in this version. :/
> 
> regards,
> dan carpenter
> 
> .
> 

On Mon, Dec 09, 2024 at 02:27:58PM +0800, Zhihao Cheng wrote:
> 在 2024/12/8 1:05, Dan Carpenter 写道:
> > On Sat, Dec 07, 2024 at 12:17:33PM +0800, Zhihao Cheng wrote:
> > > 在 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.
> > > 
> > 
> > I've looked at this again, and I still don't see the bug before the
> > commit.  Secondly, commit a1eda864c04c ("mtdchar: prevent integer
> > overflow in a safety check") is missing a Fixes tag but the message says
> > that it's this commit which introduced the bug.
> 
> Ah, I see. There is not an addition operation for 'req.start' and 'req.len'
> until commit 6420ac0af95d("mtdchar: prevent unbounded allocation in MEMWRITE
> ioctl") and 095bb6e44eb1("mtdchar: add MEMREAD ioctl"), so I guess the there
> should be two fix tags?

Ah, sure.  I can tag both those commits.

regards,
dan carpenter