| Message ID | 20220128064054.2434068-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [4.9] can: bcm: fix UAF of bcm op | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Fri, Jan 28, 2022 at 02:40:54PM +0800, Ziyang Xuan wrote: > Stopping tasklet and hrtimer rely on the active state of tasklet and > hrtimer sequentially in bcm_remove_op(), the op object will be freed > if they are all unactive. Assume the hrtimer timeout is short, the > hrtimer cb has been excuted after tasklet conditional judgment which > must be false after last round tasklet_kill() and before condition > hrtimer_active(), it is false when execute to hrtimer_active(). Bug > is triggerd, because the stopping action is end and the op object > will be freed, but the tasklet is scheduled. The resources of the op > object will occur UAF bug. > > Move hrtimer_cancel() behind tasklet_kill() and switch 'while () {...}' > to 'do {...} while ()' to fix the op UAF problem. > > Fixes: a06393ed0316 ("can: bcm: fix hrtimer/tasklet termination in bcm op removal") > Reported-by: syzbot+5ca851459ed04c778d1d@syzkaller.appspotmail.com > Cc: stable@vger.kernel.org > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > net/can/bcm.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) Both now queued up, thanks. greg k-h
diff --git a/net/can/bcm.c b/net/can/bcm.c index 369326715b9c..bfb507223468 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -761,21 +761,21 @@ static struct bcm_op *bcm_find_op(struct list_head *ops, static void bcm_remove_op(struct bcm_op *op) { if (op->tsklet.func) { - while (test_bit(TASKLET_STATE_SCHED, &op->tsklet.state) || - test_bit(TASKLET_STATE_RUN, &op->tsklet.state) || - hrtimer_active(&op->timer)) { - hrtimer_cancel(&op->timer); + do { tasklet_kill(&op->tsklet); - } + hrtimer_cancel(&op->timer); + } while (test_bit(TASKLET_STATE_SCHED, &op->tsklet.state) || + test_bit(TASKLET_STATE_RUN, &op->tsklet.state) || + hrtimer_active(&op->timer)); } if (op->thrtsklet.func) { - while (test_bit(TASKLET_STATE_SCHED, &op->thrtsklet.state) || - test_bit(TASKLET_STATE_RUN, &op->thrtsklet.state) || - hrtimer_active(&op->thrtimer)) { - hrtimer_cancel(&op->thrtimer); + do { tasklet_kill(&op->thrtsklet); - } + hrtimer_cancel(&op->thrtimer); + } while (test_bit(TASKLET_STATE_SCHED, &op->thrtsklet.state) || + test_bit(TASKLET_STATE_RUN, &op->thrtsklet.state) || + hrtimer_active(&op->thrtimer)); } if ((op->frames) && (op->frames != &op->sframe))
Stopping tasklet and hrtimer rely on the active state of tasklet and hrtimer sequentially in bcm_remove_op(), the op object will be freed if they are all unactive. Assume the hrtimer timeout is short, the hrtimer cb has been excuted after tasklet conditional judgment which must be false after last round tasklet_kill() and before condition hrtimer_active(), it is false when execute to hrtimer_active(). Bug is triggerd, because the stopping action is end and the op object will be freed, but the tasklet is scheduled. The resources of the op object will occur UAF bug. Move hrtimer_cancel() behind tasklet_kill() and switch 'while () {...}' to 'do {...} while ()' to fix the op UAF problem. Fixes: a06393ed0316 ("can: bcm: fix hrtimer/tasklet termination in bcm op removal") Reported-by: syzbot+5ca851459ed04c778d1d@syzkaller.appspotmail.com Cc: stable@vger.kernel.org Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/can/bcm.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)