[PATCH 4.14] can: bcm: fix UAF of bcm op

[PATCH 4.14] can: bcm: fix UAF of bcm op

[Ziyang Xuan]

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

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 324c4cdc003e..b3f3b02ffd42 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -762,21 +762,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))
-- 
2.25.1

Re: [PATCH 4.14] can: bcm: fix UAF of bcm op

[Greg KH]

On Sat, Jan 22, 2022 at 06:25:06PM +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(-)

What is the git commit id of this change in Linus's tree?

thanks,

greg k-h

Re: [PATCH 4.14] can: bcm: fix UAF of bcm op

[Oliver Hartkopp]

On 22.01.22 11:30, Greg KH wrote:
> On Sat, Jan 22, 2022 at 06:25:06PM +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(-)
> 
> What is the git commit id of this change in Linus's tree?

Linus' tree has been fixed by removing the tasklet implementation and 
replacing it with a HRTIMER_MODE_SOFT approach here:

commit bf74aa86e111a ("can: bcm: switch timer to HRTIMER_MODE_SOFT and 
remove hrtimer_tasklet")

This patch from Ziyang Xuan fixes the 'old' tasklet implementation for 
'old' stable kernels that lack the HRTIMER_MODE_SOFT infrastructure.

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Best regards,
Oliver




> 
> thanks,
> 
> greg k-h

Re: [PATCH 4.14] can: bcm: fix UAF of bcm op

[Greg KH]

On Sun, Jan 23, 2022 at 03:10:01PM +0100, Oliver Hartkopp wrote:
> 
> 
> On 22.01.22 11:30, Greg KH wrote:
> > On Sat, Jan 22, 2022 at 06:25:06PM +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(-)
> > 
> > What is the git commit id of this change in Linus's tree?
> 
> Linus' tree has been fixed by removing the tasklet implementation and
> replacing it with a HRTIMER_MODE_SOFT approach here:
> 
> commit bf74aa86e111a ("can: bcm: switch timer to HRTIMER_MODE_SOFT and
> remove hrtimer_tasklet")
> 
> This patch from Ziyang Xuan fixes the 'old' tasklet implementation for 'old'
> stable kernels that lack the HRTIMER_MODE_SOFT infrastructure.
> 
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks, I'll queue this up for 4.14.

Ziyang, can I get a version for 4.19.y as well?

thanks,

greg k-h