Hi Colin,
Thank you for catching this.
On Tuesday, June 22, 2021 at 8:28 AM, Colin Ian King <colin.king@xxxxxxxxxxxxx>
> 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@xxxxxxx>
> 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
