linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* re: watchdog: Add hrtimer-based pretimeout feature
@ 2021-06-22 15:27 Colin Ian King
  2021-06-22 19:41 ` Klein, Curtis
  0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2021-06-22 15:27 UTC (permalink / raw)
  To: Curtis Klein
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, linux-kernel

Hi,

Static analysis using Coverity on linux-next has found an issue in
function watchdog_cdev_unregister in source
drivers/watchdog/watchdog_dev.c with the following commit:

commit 7b7d2fdc8c3e3f9fdb3558d674e1eeddc16c7d9e
Author: Curtis Klein <curtis.klein@hpe.com>
Date:   Wed Feb 3 12:11:30 2021 -0800

    watchdog: Add hrtimer-based pretimeout feature

The analysis is as follows:

1084 static void (struct watchdog_device *wdd)
1085 {
1086        struct watchdog_core_data *wd_data = wdd->wd_data;
1087
1088        cdev_device_del(&wd_data->cdev, &wd_data->dev);

    1. Condition wdd->id == 0, taking true branch.

1089        if (wdd->id == 0) {
1090                misc_deregister(&watchdog_miscdev);
1091                old_wd_data = NULL;
1092        }
1093

    2. Condition watchdog_active(wdd), taking true branch.
    3. Condition test_bit(4, &wdd->status), taking true branch.

1094        if (watchdog_active(wdd) &&
1095            test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
1096                watchdog_stop(wdd);
1097        }
1098
1099        mutex_lock(&wd_data->lock);
1100        wd_data->wdd = NULL;

    4. assign_zero: Assigning: wdd->wd_data = NULL.

1101        wdd->wd_data = NULL;
1102        mutex_unlock(&wd_data->lock);
1103
1104        hrtimer_cancel(&wd_data->timer);
1105        kthread_cancel_work_sync(&wd_data->work);

Explicit null dereferenced (FORWARD_NULL)

    5. var_deref_model: Passing wdd to watchdog_hrtimer_pretimeout_stop,
which dereferences null wdd->wd_data.

1106        watchdog_hrtimer_pretimeout_stop(wdd);
1107
1108        put_device(&wd_data->dev);
1109 }

The call to watchdog_hrtimer_pretimeout_stop dereferences wdd as follows:

41 void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd)
42 {

  1. deref_parm_field_in_call: Function hrtimer_cancel dereferences an
offset off wdd->wd_data.

43        hrtimer_cancel(&wdd->wd_data->pretimeout_timer);
44 }

Since wdd->wd_data is set to NULL on line 1101, the call to
watchdog_hrtimer_pretimeout_stop will always trip a null pointer
dereference.  Shall we just remove that call?

Colin

^ permalink raw reply	[flat|nested] 2+ messages in thread

* RE: watchdog: Add hrtimer-based pretimeout feature
  2021-06-22 15:27 watchdog: Add hrtimer-based pretimeout feature Colin Ian King
@ 2021-06-22 19:41 ` Klein, Curtis
  0 siblings, 0 replies; 2+ messages in thread
From: Klein, Curtis @ 2021-06-22 19:41 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, linux-kernel

Hi Colin,

Thank you for catching this.

On Tuesday, June 22, 2021 at 8:28 AM, Colin Ian King <colin.king@canonical.com>
> Hi,
> 
> Static analysis using Coverity on linux-next has found an issue in
> function watchdog_cdev_unregister in source
> drivers/watchdog/watchdog_dev.c with the following commit:
> 
> commit 7b7d2fdc8c3e3f9fdb3558d674e1eeddc16c7d9e
> Author: Curtis Klein <curtis.klein@hpe.com>
> Date:   Wed Feb 3 12:11:30 2021 -0800
> 
>     watchdog: Add hrtimer-based pretimeout feature
> 
> The analysis is as follows:
> 
> 1084 static void (struct watchdog_device *wdd)
> 1085 {
> 1086        struct watchdog_core_data *wd_data = wdd->wd_data;
> 1087
> 1088        cdev_device_del(&wd_data->cdev, &wd_data->dev);
> 
>     1. Condition wdd->id == 0, taking true branch.
> 
> 1089        if (wdd->id == 0) {
> 1090                misc_deregister(&watchdog_miscdev);
> 1091                old_wd_data = NULL;
> 1092        }
> 1093
> 
>     2. Condition watchdog_active(wdd), taking true branch.
>     3. Condition test_bit(4, &wdd->status), taking true branch.
> 
> 1094        if (watchdog_active(wdd) &&
> 1095            test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
> 1096                watchdog_stop(wdd);
> 1097        }
> 1098
> 1099        mutex_lock(&wd_data->lock);
> 1100        wd_data->wdd = NULL;
> 
>     4. assign_zero: Assigning: wdd->wd_data = NULL.
> 
> 1101        wdd->wd_data = NULL;
> 1102        mutex_unlock(&wd_data->lock);
> 1103
> 1104        hrtimer_cancel(&wd_data->timer);
> 1105        kthread_cancel_work_sync(&wd_data->work);
> 
> Explicit null dereferenced (FORWARD_NULL)
> 
>     5. var_deref_model: Passing wdd to watchdog_hrtimer_pretimeout_stop,
> which dereferences null wdd->wd_data.
> 
> 1106        watchdog_hrtimer_pretimeout_stop(wdd);
> 1107
> 1108        put_device(&wd_data->dev);
> 1109 }
> 
> The call to watchdog_hrtimer_pretimeout_stop dereferences wdd as
> follows:
> 
> 41 void watchdog_hrtimer_pretimeout_stop(struct watchdog_device *wdd)
> 42 {
> 
>   1. deref_parm_field_in_call: Function hrtimer_cancel dereferences an
> offset off wdd->wd_data.
> 
> 43        hrtimer_cancel(&wdd->wd_data->pretimeout_timer);
> 44 }
> 
> Since wdd->wd_data is set to NULL on line 1101, the call to
> watchdog_hrtimer_pretimeout_stop will always trip a null pointer
> dereference.  Shall we just remove that call?

Yes, it can be removed. It's actually redundant since the call to watchdog_stop
on line 1089 will call watchdog_hrtimer_pretimeout_stop.

I will send out a fix today.

>
> Colin

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-06-22 19:42 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 15:27 watchdog: Add hrtimer-based pretimeout feature Colin Ian King
2021-06-22 19:41 ` Klein, Curtis

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).