On Tue, Jan 18, 2022 at 06:32:39PM +0200, Ovidiu Panait wrote:
> From: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
>
> commit ba316be1b6a00db7126ed9a39f9bee434a508043 upstream.
>
> struct sock.sk_timer should be used as a sock cleanup timer. However,
> SCO uses it to implement sock timeouts.
>
> This causes issues because struct sock.sk_timer's callback is run in
> an IRQ context, and the timer callback function sco_sock_timeout takes
> a spin lock on the socket. However, other functions such as
> sco_conn_del and sco_conn_ready take the spin lock with interrupts
> enabled.
>
> This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
> lead to deadlocks as reported by Syzbot [1]:
> CPU0
> ----
> lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> <Interrupt>
> lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
>
> To fix this, we use delayed work to implement SCO sock timouts
> instead. This allows us to avoid taking the spin lock on the socket in
> an IRQ context, and corrects the misuse of struct sock.sk_timer.
>
> As a note, cancel_delayed_work is used instead of
> cancel_delayed_work_sync in sco_sock_set_timer and
> sco_sock_clear_timer to avoid a deadlock. In the future, the call to
> bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
> synchronize with other functions using lock_sock. However, since
> sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
> the locked socket (in sco_connect and __sco_sock_close),
> cancel_delayed_work_sync might cause them to sleep until an
> sco_sock_timeout that has started finishes running. But
> sco_sock_timeout would also sleep until it can grab the lock_sock.
>
> Using cancel_delayed_work is fine because sco_sock_timeout does not
> change from run to run, hence there is no functional difference
> between:
> 1. waiting for a timeout to finish running before scheduling another
> timeout
> 2. scheduling another timeout while a timeout is running.
>
> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
> Reported-by: syzbot+2f6d7c28bb4bf7e82060@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: syzbot+2f6d7c28bb4bf7e82060@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> [OP: adjusted context for 4.14]
> Signed-off-by: Ovidiu Panait <ovidiu.panait@xxxxxxxxxxxxx>
> ---
> Note: these 2 fixes are part of CVE-2021-3640 patchset and are already present
> in 4.14+ stable branches. For this backport, small contextual adjustments
> were done to account for the old setup_timer() API usage.
Both now queued up, thanks.
greg k-h
