Re: [PATCH] mtd: phram: Prevent divide by zero bug in phram_setup()

[Miquel Raynal <miquel.raynal@bootlin.com>]

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