All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
@ 2022-01-21  5:38 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2022-01-21  5:38 UTC (permalink / raw)
  To: Joern Engel, yangerkun
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

The problem is that "erasesize" is a uint32_t type so it might be
non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
would lead to the divide by zero bug.

Avoid the bug by delaying the divide until after we have validated
that "erasesize" is reasonable.

Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mtd/devices/phram.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 6ed6c51fac69..d503821a3e60 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -264,16 +264,20 @@ static int phram_setup(const char *val)
 		}
 	}
 
-	if (erasesize)
-		div_u64_rem(len, (uint32_t)erasesize, &rem);
-
 	if (len == 0 || erasesize == 0 || erasesize > len
-	    || erasesize > UINT_MAX || rem) {
+	    || erasesize > UINT_MAX) {
 		parse_err("illegal erasesize or len\n");
 		ret = -EINVAL;
 		goto error;
 	}
 
+	div_u64_rem(len, (uint32_t)erasesize, &rem);
+	if (rem) {
+		parse_err("len is not multiple of erasesize\n");
+		ret = -EINVAL;
+		goto error;
+	}
+
 	ret = register_device(name, start, len, (uint32_t)erasesize);
 	if (ret)
 		goto error;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
@ 2022-01-21  5:38 ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2022-01-21  5:38 UTC (permalink / raw)
  To: Joern Engel, yangerkun
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

The problem is that "erasesize" is a uint32_t type so it might be
non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
would lead to the divide by zero bug.

Avoid the bug by delaying the divide until after we have validated
that "erasesize" is reasonable.

Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mtd/devices/phram.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 6ed6c51fac69..d503821a3e60 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -264,16 +264,20 @@ static int phram_setup(const char *val)
 		}
 	}
 
-	if (erasesize)
-		div_u64_rem(len, (uint32_t)erasesize, &rem);
-
 	if (len == 0 || erasesize == 0 || erasesize > len
-	    || erasesize > UINT_MAX || rem) {
+	    || erasesize > UINT_MAX) {
 		parse_err("illegal erasesize or len\n");
 		ret = -EINVAL;
 		goto error;
 	}
 
+	div_u64_rem(len, (uint32_t)erasesize, &rem);
+	if (rem) {
+		parse_err("len is not multiple of erasesize\n");
+		ret = -EINVAL;
+		goto error;
+	}
+
 	ret = register_device(name, start, len, (uint32_t)erasesize);
 	if (ret)
 		goto error;
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
  2022-01-21  5:38 ` Dan Carpenter
@ 2022-01-21 11:17   ` Miquel Raynal
  -1 siblings, 0 replies; 8+ messages in thread
From: Miquel Raynal @ 2022-01-21 11:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Joern Engel, yangerkun, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

Hi Dan,

dan.carpenter@oracle.com wrote on Fri, 21 Jan 2022 08:38:36 +0300:

> The problem is that "erasesize" is a uint32_t type so it might be

Don't you mean uint64_t here? Otherwise I don't get the sentence.

> non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That

s/*is*/*can* be/ ? (again, if my understanding is correct).

> would lead to the divide by zero bug.
> 
> Avoid the bug by delaying the divide until after we have validated
> that "erasesize" is reasonable.

I don't really get the fix. If "erasesize" is big enough, then
(uint32_t)erasesize can however be zero. But checking if erasesize is
zero beforehands does not fix the situation. Or am I missing
something?

> Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/mtd/devices/phram.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 6ed6c51fac69..d503821a3e60 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -264,16 +264,20 @@ static int phram_setup(const char *val)
>  		}
>  	}
>  
> -	if (erasesize)
> -		div_u64_rem(len, (uint32_t)erasesize, &rem);
> -
>  	if (len == 0 || erasesize == 0 || erasesize > len
> -	    || erasesize > UINT_MAX || rem) {
> +	    || erasesize > UINT_MAX) {
>  		parse_err("illegal erasesize or len\n");
>  		ret = -EINVAL;
>  		goto error;
>  	}
>  
> +	div_u64_rem(len, (uint32_t)erasesize, &rem);
> +	if (rem) {
> +		parse_err("len is not multiple of erasesize\n");
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
>  	ret = register_device(name, start, len, (uint32_t)erasesize);
>  	if (ret)
>  		goto error;


Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
@ 2022-01-21 11:17   ` Miquel Raynal
  0 siblings, 0 replies; 8+ messages in thread
From: Miquel Raynal @ 2022-01-21 11:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Joern Engel, yangerkun, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

Hi Dan,

dan.carpenter@oracle.com wrote on Fri, 21 Jan 2022 08:38:36 +0300:

> The problem is that "erasesize" is a uint32_t type so it might be

Don't you mean uint64_t here? Otherwise I don't get the sentence.

> non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That

s/*is*/*can* be/ ? (again, if my understanding is correct).

> would lead to the divide by zero bug.
> 
> Avoid the bug by delaying the divide until after we have validated
> that "erasesize" is reasonable.

I don't really get the fix. If "erasesize" is big enough, then
(uint32_t)erasesize can however be zero. But checking if erasesize is
zero beforehands does not fix the situation. Or am I missing
something?

> Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/mtd/devices/phram.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 6ed6c51fac69..d503821a3e60 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -264,16 +264,20 @@ static int phram_setup(const char *val)
>  		}
>  	}
>  
> -	if (erasesize)
> -		div_u64_rem(len, (uint32_t)erasesize, &rem);
> -
>  	if (len == 0 || erasesize == 0 || erasesize > len
> -	    || erasesize > UINT_MAX || rem) {
> +	    || erasesize > UINT_MAX) {
>  		parse_err("illegal erasesize or len\n");
>  		ret = -EINVAL;
>  		goto error;
>  	}
>  
> +	div_u64_rem(len, (uint32_t)erasesize, &rem);
> +	if (rem) {
> +		parse_err("len is not multiple of erasesize\n");
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
>  	ret = register_device(name, start, len, (uint32_t)erasesize);
>  	if (ret)
>  		goto error;


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
  2022-01-21 11:17   ` Miquel Raynal
@ 2022-01-21 11:51     ` Dan Carpenter
  -1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2022-01-21 11:51 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Joern Engel, yangerkun, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

On Fri, Jan 21, 2022 at 12:17:48PM +0100, Miquel Raynal wrote:
> Hi Dan,
> 
> dan.carpenter@oracle.com wrote on Fri, 21 Jan 2022 08:38:36 +0300:
> 
> > The problem is that "erasesize" is a uint32_t type so it might be
> 
> Don't you mean uint64_t here? Otherwise I don't get the sentence.
> 

Yeah.  I meant uint64_t.  Will resend.


> > non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
> 
> s/*is*/*can* be/ ? (again, if my understanding is correct).
> 

It might be a situation where "erasesize" is non-zero but after we
truncated it "(uint32_t)erasesize" is zero.

> > would lead to the divide by zero bug.
> > 
> > Avoid the bug by delaying the divide until after we have validated
> > that "erasesize" is reasonable.
> 
> I don't really get the fix. If "erasesize" is big enough, then
> (uint32_t)erasesize can however be zero. But checking if erasesize is
> zero beforehands does not fix the situation. Or am I missing
> something?

It doesn't just check for zero, it checks a couple other things
including  if erasesize > UINT_MAX.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
@ 2022-01-21 11:51     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2022-01-21 11:51 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Joern Engel, yangerkun, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

On Fri, Jan 21, 2022 at 12:17:48PM +0100, Miquel Raynal wrote:
> Hi Dan,
> 
> dan.carpenter@oracle.com wrote on Fri, 21 Jan 2022 08:38:36 +0300:
> 
> > The problem is that "erasesize" is a uint32_t type so it might be
> 
> Don't you mean uint64_t here? Otherwise I don't get the sentence.
> 

Yeah.  I meant uint64_t.  Will resend.


> > non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
> 
> s/*is*/*can* be/ ? (again, if my understanding is correct).
> 

It might be a situation where "erasesize" is non-zero but after we
truncated it "(uint32_t)erasesize" is zero.

> > would lead to the divide by zero bug.
> > 
> > Avoid the bug by delaying the divide until after we have validated
> > that "erasesize" is reasonable.
> 
> I don't really get the fix. If "erasesize" is big enough, then
> (uint32_t)erasesize can however be zero. But checking if erasesize is
> zero beforehands does not fix the situation. Or am I missing
> something?

It doesn't just check for zero, it checks a couple other things
including  if erasesize > UINT_MAX.

regards,
dan carpenter


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
  2022-01-21  5:38 ` Dan Carpenter
@ 2022-01-22  2:43   ` yangerkun
  -1 siblings, 0 replies; 8+ messages in thread
From: yangerkun @ 2022-01-22  2:43 UTC (permalink / raw)
  To: Dan Carpenter, Joern Engel
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

Thanks for the fix!

Reviewed-by: yangerkun <yangerkun@huawei.com>

On 2022/1/21 13:38, Dan Carpenter wrote:
> The problem is that "erasesize" is a uint32_t type so it might be
> non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
> would lead to the divide by zero bug.
> 
> Avoid the bug by delaying the divide until after we have validated
> that "erasesize" is reasonable.
> 
> Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/mtd/devices/phram.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 6ed6c51fac69..d503821a3e60 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -264,16 +264,20 @@ static int phram_setup(const char *val)
>   		}
>   	}
>   
> -	if (erasesize)
> -		div_u64_rem(len, (uint32_t)erasesize, &rem);
> -
>   	if (len == 0 || erasesize == 0 || erasesize > len
> -	    || erasesize > UINT_MAX || rem) {
> +	    || erasesize > UINT_MAX) {
>   		parse_err("illegal erasesize or len\n");
>   		ret = -EINVAL;
>   		goto error;
>   	}
>   
> +	div_u64_rem(len, (uint32_t)erasesize, &rem);
> +	if (rem) {
> +		parse_err("len is not multiple of erasesize\n");
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
>   	ret = register_device(name, start, len, (uint32_t)erasesize);
>   	if (ret)
>   		goto error;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()
@ 2022-01-22  2:43   ` yangerkun
  0 siblings, 0 replies; 8+ messages in thread
From: yangerkun @ 2022-01-22  2:43 UTC (permalink / raw)
  To: Dan Carpenter, Joern Engel
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, kernel-janitors

Thanks for the fix!

Reviewed-by: yangerkun <yangerkun@huawei.com>

On 2022/1/21 13:38, Dan Carpenter wrote:
> The problem is that "erasesize" is a uint32_t type so it might be
> non-zero but the truncated "(uint32_t)erasesize" value *is* zero. That
> would lead to the divide by zero bug.
> 
> Avoid the bug by delaying the divide until after we have validated
> that "erasesize" is reasonable.
> 
> Fixes: dc2b3e5cbc80 ("mtd: phram: use div_u64_rem to stop overwrite len in phram_setup")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   drivers/mtd/devices/phram.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 6ed6c51fac69..d503821a3e60 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -264,16 +264,20 @@ static int phram_setup(const char *val)
>   		}
>   	}
>   
> -	if (erasesize)
> -		div_u64_rem(len, (uint32_t)erasesize, &rem);
> -
>   	if (len == 0 || erasesize == 0 || erasesize > len
> -	    || erasesize > UINT_MAX || rem) {
> +	    || erasesize > UINT_MAX) {
>   		parse_err("illegal erasesize or len\n");
>   		ret = -EINVAL;
>   		goto error;
>   	}
>   
> +	div_u64_rem(len, (uint32_t)erasesize, &rem);
> +	if (rem) {
> +		parse_err("len is not multiple of erasesize\n");
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
>   	ret = register_device(name, start, len, (uint32_t)erasesize);
>   	if (ret)
>   		goto error;
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-22  2:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-21  5:38 [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup() Dan Carpenter
2022-01-21  5:38 ` Dan Carpenter
2022-01-21 11:17 ` Miquel Raynal
2022-01-21 11:17   ` Miquel Raynal
2022-01-21 11:51   ` Dan Carpenter
2022-01-21 11:51     ` Dan Carpenter
2022-01-22  2:43 ` yangerkun
2022-01-22  2:43   ` yangerkun

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.