[V2,next] rtc: ds1307: check for failed memory allocation on wdt

From: Colin Ian King <colin.king@canonical.com>

Currently a failed memory allocation will lead to a null pointer
dereference on point wdt.  Fix this by checking for a failed allocation
and adding error return handling to function ds1307_wdt_register.
Also move the error exit label "exit" to allow a return statement to
be removed.

Addresses-Coverity: ("Dereference null return")
Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
V2: move error exit label and remove a return statement, thanks to 
    Walter Harms for spotting this clean up.
---
 drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt.  Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
> 
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to 
>     Walter Harms for spotting this clean up.
> ---
>  drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>  
>  };
>  
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)

What exactly is the point of returning an error just to ignore it ?

Guenter

>  {
>  	struct watchdog_device	*wdt;
>  
>  	if (ds1307->type != ds_1388)
> -		return;
> +		return 0;
>  
>  	wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
>  
>  	wdt->info = &ds1388_wdt_info;
>  	wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
>  	watchdog_init_timeout(wdt, 0, ds1307->dev);
>  	watchdog_set_drvdata(wdt, ds1307);
>  	devm_watchdog_register_device(ds1307->dev, wdt);
> +
> +	return 0;
>  }
>  #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
>  {
> +	return 0;
>  }
>  #endif /* CONFIG_WATCHDOG_CORE */
>  
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>  
>  	ds1307_hwmon_register(ds1307);
>  	ds1307_clks_register(ds1307);
> -	ds1307_wdt_register(ds1307);
> -
> -	return 0;
> -
> +	err = ds1307_wdt_register(ds1307);
>  exit:
>  	return err;
>  }
>

On 4/2/20 6:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt.  Fix this by checking for a failed allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
> 
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to 
>     Walter Harms for spotting this clean up.
> ---
>  drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
>  
>  };
>  
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
>  {
>  	struct watchdog_device	*wdt;
>  
>  	if (ds1307->type != ds_1388)
> -		return;
> +		return 0;
>  
>  	wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
>  
>  	wdt->info = &ds1388_wdt_info;
>  	wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
>  	watchdog_init_timeout(wdt, 0, ds1307->dev);
>  	watchdog_set_drvdata(wdt, ds1307);
>  	devm_watchdog_register_device(ds1307->dev, wdt);
> +
> +	return 0;
>  }
>  #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
>  {
> +	return 0;
>  }
>  #endif /* CONFIG_WATCHDOG_CORE */
>  
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
>  
>  	ds1307_hwmon_register(ds1307);
>  	ds1307_clks_register(ds1307);
> -	ds1307_wdt_register(ds1307);
> -
> -	return 0;
> -
> +	err = ds1307_wdt_register(ds1307);

Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
Same as with hwmon. If you want to change this, fine with me. Note though
that rtc_nvmem_register() now leaks a sysfs file if I understand the code
correctly.

Guenter

>  exit:
>  	return err;
>  }
>

On 02/04/2020 07:44:44-0700, Guenter Roeck wrote:
> On 4/2/20 6:52 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently a failed memory allocation will lead to a null pointer
> > dereference on point wdt.  Fix this by checking for a failed allocation
> > and adding error return handling to function ds1307_wdt_register.
> > Also move the error exit label "exit" to allow a return statement to
> > be removed.
> > 
> > Addresses-Coverity: ("Dereference null return")
> > Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on ds1388")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > V2: move error exit label and remove a return statement, thanks to 
> >     Walter Harms for spotting this clean up.
> > ---
> >  drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> > index fad042118862..c058b02efb4d 100644
> > --- a/drivers/rtc/rtc-ds1307.c
> > +++ b/drivers/rtc/rtc-ds1307.c
> > @@ -1665,14 +1665,16 @@ static const struct watchdog_ops ds1388_wdt_ops = {
> >  
> >  };
> >  
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> >  {
> >  	struct watchdog_device	*wdt;
> >  
> >  	if (ds1307->type != ds_1388)
> > -		return;
> > +		return 0;
> >  
> >  	wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> >  
> >  	wdt->info = &ds1388_wdt_info;
> >  	wdt->ops = &ds1388_wdt_ops;
> > @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307 *ds1307)
> >  	watchdog_init_timeout(wdt, 0, ds1307->dev);
> >  	watchdog_set_drvdata(wdt, ds1307);
> >  	devm_watchdog_register_device(ds1307->dev, wdt);
> > +
> > +	return 0;
> >  }
> >  #else
> > -static void ds1307_wdt_register(struct ds1307 *ds1307)
> > +static int ds1307_wdt_register(struct ds1307 *ds1307)
> >  {
> > +	return 0;
> >  }
> >  #endif /* CONFIG_WATCHDOG_CORE */
> >  
> > @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client *client,
> >  
> >  	ds1307_hwmon_register(ds1307);
> >  	ds1307_clks_register(ds1307);
> > -	ds1307_wdt_register(ds1307);
> > -
> > -	return 0;
> > -
> > +	err = ds1307_wdt_register(ds1307);
> 
> Ah, sorry, missed this one. The original idea was to ignore errors on purpose.
> Same as with hwmon. If you want to change this, fine with me. Note though
> that rtc_nvmem_register() now leaks a sysfs file if I understand the code
> correctly.

rtc_nvmem_unregister(rtc) should be called properly by
devm_rtc_release_device when the rtc_device is destroyed so I don't
think it leaks.

As stated, I also prefer keeping the watchdog optional and ignore the
error.

On Thu, 2020-04-02 at 14:52 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently a failed memory allocation will lead to a null pointer
> dereference on point wdt.  Fix this by checking for a failed
> allocation
> and adding error return handling to function ds1307_wdt_register.
> Also move the error exit label "exit" to allow a return statement to
> be removed.
> 
> Addresses-Coverity: ("Dereference null return")
> Fixes: fd90d48db037 ("rtc: ds1307: add support for watchdog timer on
> ds1388")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: move error exit label and remove a return statement, thanks to 
>     Walter Harms for spotting this clean up.
> ---
>  drivers/rtc/rtc-ds1307.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index fad042118862..c058b02efb4d 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -1665,14 +1665,16 @@ static const struct watchdog_ops
> ds1388_wdt_ops = {
>  
>  };
>  
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
>  {
>  	struct watchdog_device	*wdt;
>  
>  	if (ds1307->type != ds_1388)
> -		return;
> +		return 0;
>  
>  	wdt = devm_kzalloc(ds1307->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;

My original intention was that the wdt support was optional. I'd
suggest just

+	if (!wdt)
+		return;

Which should keep Coverity happy.

>  	wdt->info = &ds1388_wdt_info;
>  	wdt->ops = &ds1388_wdt_ops;
> @@ -1683,10 +1685,13 @@ static void ds1307_wdt_register(struct ds1307
> *ds1307)
>  	watchdog_init_timeout(wdt, 0, ds1307->dev);
>  	watchdog_set_drvdata(wdt, ds1307);
>  	devm_watchdog_register_device(ds1307->dev, wdt);
> +
> +	return 0;
>  }
>  #else
> -static void ds1307_wdt_register(struct ds1307 *ds1307)
> +static int ds1307_wdt_register(struct ds1307 *ds1307)
>  {
> +	return 0;
>  }
>  #endif /* CONFIG_WATCHDOG_CORE */
>  
> @@ -1979,10 +1984,7 @@ static int ds1307_probe(struct i2c_client
> *client,
>  
>  	ds1307_hwmon_register(ds1307);
>  	ds1307_clks_register(ds1307);
> -	ds1307_wdt_register(ds1307);
> -
> -	return 0;
> -
> +	err = ds1307_wdt_register(ds1307);
>  exit:
>  	return err;
>  }

On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
> 
> As stated, I also prefer keeping the watchdog optional and ignore the
> error.

Hopefully you aren't running out of memory on start up.  In current
kernels small memory allocations like this never fail so it doesn't
really affect runtime.  It only silences the NULL dereference static
checker warning.

regards,
dan carpenter

On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
> > 
> > As stated, I also prefer keeping the watchdog optional and ignore the
> > error.
> 
> Hopefully you aren't running out of memory on start up.  In current
> kernels small memory allocations like this never fail so it doesn't
> really affect runtime.  It only silences the NULL dereference static
> checker warning.
> 

Yes, so the

if (!wdt)
 return;

would be enough instead of introducing unnecessary error handling.

On 03/04/2020 10:22, Alexandre Belloni wrote:
> On 03/04/2020 11:45:04+0300, Dan Carpenter wrote:
>> On Thu, Apr 02, 2020 at 04:53:12PM +0200, Alexandre Belloni wrote:
>>>
>>> As stated, I also prefer keeping the watchdog optional and ignore the
>>> error.
>>
>> Hopefully you aren't running out of memory on start up.  In current
>> kernels small memory allocations like this never fail so it doesn't
>> really affect runtime.  It only silences the NULL dereference static
>> checker warning.
>>
> 
> Yes, so the
> 
> if (!wdt)
>  return;
> 
> would be enough instead of introducing unnecessary error handling.
> 
OK, I'll send a V3 later today.