* [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
@ 2020-04-02 13:52 Colin King
2020-04-02 14:39 ` Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Colin King @ 2020-04-02 13:52 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni, Guenter Roeck,
Chris Packham, linux-rtc
Cc: kernel-janitors, linux-kernel
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);
exit:
return err;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt Colin King
@ 2020-04-02 14:39 ` Guenter Roeck
2020-04-02 14:44 ` Guenter Roeck
2020-04-02 19:51 ` Chris Packham
2 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:39 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt Colin King
2020-04-02 14:39 ` Guenter Roeck
@ 2020-04-02 14:44 ` Guenter Roeck
2020-04-02 14:53 ` Alexandre Belloni
2020-04-02 19:51 ` Chris Packham
2 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-04-02 14:44 UTC (permalink / raw)
To: Colin King, Alessandro Zummo, Alexandre Belloni, Chris Packham,
linux-rtc
Cc: kernel-janitors, linux-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 14:44 ` Guenter Roeck
@ 2020-04-02 14:53 ` Alexandre Belloni
2020-04-03 8:45 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2020-04-02 14:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: Colin King, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
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.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 13:52 [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt Colin King
2020-04-02 14:39 ` Guenter Roeck
2020-04-02 14:44 ` Guenter Roeck
@ 2020-04-02 19:51 ` Chris Packham
2 siblings, 0 replies; 8+ messages in thread
From: Chris Packham @ 2020-04-02 19:51 UTC (permalink / raw)
To: linux, colin.king, a.zummo, linux-rtc, alexandre.belloni
Cc: linux-kernel, kernel-janitors
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;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-02 14:53 ` Alexandre Belloni
@ 2020-04-03 8:45 ` Dan Carpenter
2020-04-03 9:22 ` Alexandre Belloni
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-04-03 8:45 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-03 8:45 ` Dan Carpenter
@ 2020-04-03 9:22 ` Alexandre Belloni
2020-04-03 10:01 ` Colin Ian King
0 siblings, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2020-04-03 9:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Guenter Roeck, Colin King, Alessandro Zummo, Chris Packham,
linux-rtc, kernel-janitors, linux-kernel
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.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt
2020-04-03 9:22 ` Alexandre Belloni
@ 2020-04-03 10:01 ` Colin Ian King
0 siblings, 0 replies; 8+ messages in thread
From: Colin Ian King @ 2020-04-03 10:01 UTC (permalink / raw)
To: Alexandre Belloni, Dan Carpenter
Cc: Guenter Roeck, Alessandro Zummo, Chris Packham, linux-rtc,
kernel-janitors, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-03 10:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 13:52 [PATCH][V2][next] rtc: ds1307: check for failed memory allocation on wdt Colin King
2020-04-02 14:39 ` Guenter Roeck
2020-04-02 14:44 ` Guenter Roeck
2020-04-02 14:53 ` Alexandre Belloni
2020-04-03 8:45 ` Dan Carpenter
2020-04-03 9:22 ` Alexandre Belloni
2020-04-03 10:01 ` Colin Ian King
2020-04-02 19:51 ` Chris Packham
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).