diff mbox series

[4.14,2/2] can: bcm: fix UAF of bcm op

Message ID 20220127180256.840826051@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg KH Jan. 27, 2022, 6:08 p.m. UTC 
From: Ziyang Xuan <william.xuanziyang@huawei.com>

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>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/can/bcm.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Pavel Machek Feb. 3, 2022, 8:45 p.m. UTC | #1 
Hi!


> From: Ziyang Xuan <william.xuanziyang@huawei.com>
> 
> 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.

I don't see this commit in mainline or next kernels. What is going on
here? Is it one of those "only needed in -stable" things? Or do we
still need to fix it in the mainline?

Best regards,
								Pavel
Greg KH Feb. 4, 2022, 6:59 a.m. UTC | #2 
On Thu, Feb 03, 2022 at 09:45:18PM +0100, Pavel Machek wrote:
> Hi!
> 
> 
> > From: Ziyang Xuan <william.xuanziyang@huawei.com>
> > 
> > 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.
> 
> I don't see this commit in mainline or next kernels. What is going on
> here? Is it one of those "only needed in -stable" things? Or do we
> still need to fix it in the mainline?

Please see the stable list discussion of this commit for other branches,
it should answer your question.

thanks,

greg k-h
diff mbox series

Patch

--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -762,21 +762,21 @@  static struct bcm_op *bcm_find_op(struct
 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))