| Message ID | 20220115122650.128182-1-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfc: llcp: fix and improvements | expand |
Please don't mix cleanups and bug fixes. Thank you.
On 16/01/2022 14:41, Hillf Danton wrote: > Hey Krzysztof > > On Sat, 15 Jan 2022 13:26:44 +0100 Krzysztof Kozlowski wrote: >> +++ b/net/nfc/llcp_sock.c >> @@ -789,6 +789,11 @@ static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg, >> >> lock_sock(sk); >> >> + if (!llcp_sock->local) { >> + release_sock(sk); >> + return -ENODEV; >> + } >> + >> if (sk->sk_type == SOCK_DGRAM) { >> DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr, >> msg->msg_name); >> -- >> 2.32.0 > > Given the same check for llcp local in nfc_llcp_send_ui_frame(), adding > another check does not help. Helps, because other is not protected with lock. The other could be removed, because it is simply wrong, but I did not check it. The patch fixes the report and reproducible race, but maybe does not necessarily fix entirely the race (which maybe this is what you meant by "does not help"?). Best regards, Krzysztof
On 16/01/2022 13:32, David Miller wrote: > > Please don't mix cleanups and bug fixes. The fix is the first patch, so it is easy to apply. Do you wish me to resend it? Best regards, Krzysztof
On Sun, 16 Jan 2022 17:58:28 +0100 Krzysztof Kozlowski wrote: > On 16/01/2022 13:32, David Miller wrote: > > > > Please don't mix cleanups and bug fixes. > > The fix is the first patch, so it is easy to apply. Do you wish me to > resend it? Yes, please. 99% sure Dave is expecting you to do so. FWIW the scripts I use for tag normalization and adding Links won't work when picking one patch out of entire series so repost is best. Thanks!
Hi, Patch #1: ========= Syzbot reported an easily reproducible NULL pointer dereference which I was struggling to analyze: https://syzkaller.appspot.com/bug?extid=7f23bcddf626e0593a39 Although direct fix is obvious, I could not actually find the exact race condition scenario leading to it. The patch fixes the issue - at least under my QEMU - however all this code looks racy, so I have a feeling I am plumbing one leak without fixing root cause. Therefore I would appreciate some more thoughts on first commit. The rest of patches: ==================== These are improvements, rebased on top of #1, although should be independent. They do not fix any experienced issue, just look correct to me from the code point of view. Testing ======= Under QEMU only. The NFC/LLCP code was not really tested on a device. Best regards, Krzysztof Krzysztof Kozlowski (7): nfc: llcp: fix NULL error pointer dereference on sendmsg() after failed bind() nfc: llcp: nullify llcp_sock->dev on connect() error paths nfc: llcp: simplify llcp_sock_connect() error paths nfc: llcp: use centralized exiting of bind on errors nfc: llcp: use test_bit() nfc: llcp: protect nfc_llcp_sock_unlink() calls nfc: llcp: Revert "NFC: Keep socket alive until the DISC PDU is actually sent" net/nfc/llcp.h | 1 - net/nfc/llcp_core.c | 9 +------- net/nfc/llcp_sock.c | 54 ++++++++++++++++++++++++--------------------- 3 files changed, 30 insertions(+), 34 deletions(-)