Hi Dan,
dan.carpenter@xxxxxxxxxx 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@xxxxxxxxxx>
> ---
> 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
