| Message ID | 7c198c2aa08b34045b8f9e0afe3d0b3bf5802180.1700943019.git.code@siddh.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix UAF caused by racing datagram sending and freeing of nfc_dev | expand |
On 25/11/2023 21:26, Siddh Raman Pant wrote: > llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame(), which accesses the > nfc_dev from the llcp_sock for getting the headroom and tailroom needed > for skb allocation. This path should have reference to nfc device: nfc_get_device(). Why is this not sufficient? > > Parallely, the nfc_dev can be freed via the nfc_unregister_device() > codepath (nfc_release() being called due to the class unregister in > nfc_exit()), leading to the UAF reported by Syzkaller. > > We have the following call tree before freeing: > > nfc_unregister_device() > -> nfc_llcp_unregister_device() > -> local_cleanup() > -> nfc_llcp_socket_release() > > nfc_llcp_socket_release() sets the state of sockets to LLCP_CLOSED, > and this is encountered necessarily before any freeing of nfc_dev. Sorry, I don't understand. What is encountered before freeing? > > Thus, add a rwlock in struct llcp_sock to synchronize access to > nfc_dev. nfc_dev in an llcp_sock will be NULLed in a write critical > section when socket state has been set to closed. Thus, we can avoid > the UAF by bailing out from a read critical section upon seeing NULL. > > Since this is repeated multiple times in nfc_llcp_socket_release(), > extract the behaviour into a new function. > > Reported-and-tested-by: syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d > Signed-off-by: Siddh Raman Pant <code@siddh.me> > --- > net/nfc/llcp.h | 1 + > net/nfc/llcp_commands.c | 27 ++++++++++++++++++++++++--- > net/nfc/llcp_core.c | 31 +++++++++++++++++++------------ > net/nfc/llcp_sock.c | 2 ++ > 4 files changed, 46 insertions(+), 15 deletions(-) > > diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h > index d8345ed57c95..800cbe8e3d6b 100644 > --- a/net/nfc/llcp.h > +++ b/net/nfc/llcp.h > @@ -102,6 +102,7 @@ struct nfc_llcp_local { > struct nfc_llcp_sock { > struct sock sk; > struct nfc_dev *dev; > + rwlock_t rw_dev_lock; I dislike the idea of introducing the third (!!!) lock here. It looks like a bandaid for this one particular problem. > struct nfc_llcp_local *local; > u32 target_idx; > u32 nfc_protocol; > diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c > index 39c7c59bbf66..b132830bc206 100644 > --- a/net/nfc/llcp_commands.c > +++ b/net/nfc/llcp_commands.c > @@ -315,13 +315,24 @@ static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock, > { > struct sk_buff *skb; > int err, headroom, tailroom; > + unsigned long irq_flags; > > if (sock->ssap == 0) > return NULL; > > + read_lock_irqsave(&sock->rw_dev_lock, irq_flags); > + > + if (!sock->dev) { > + read_unlock_irqrestore(&sock->rw_dev_lock, irq_flags); > + pr_err("NFC device does not exit\n"); exist? Best regards, Krzysztof
On Mon, 27 Nov 2023 16:08:16 +0530, Krzysztof Kozlowski wrote: > On 25/11/2023 21:26, Siddh Raman Pant wrote: > > llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame(), which accesses the > > nfc_dev from the llcp_sock for getting the headroom and tailroom needed > > for skb allocation. > > This path should have reference to nfc device: nfc_get_device(). Why is > this not sufficient? The index needed for nfc_get_device() is inside nfc_dev itself. Though now that I think about it, I should have modified the get and put functions of llcp_local itself to hold the ref. As you said, it looks like a band-aid with the extra lock. I agree. Sorry about that. > > Parallely, the nfc_dev can be freed via the nfc_unregister_device() > > codepath (nfc_release() being called due to the class unregister in > > nfc_exit()), leading to the UAF reported by Syzkaller. > > > > We have the following call tree before freeing: > > > > nfc_unregister_device() > > -> nfc_llcp_unregister_device() > > -> local_cleanup() > > -> nfc_llcp_socket_release() > > > > nfc_llcp_socket_release() sets the state of sockets to LLCP_CLOSED, > > and this is encountered necessarily before any freeing of nfc_dev. > > Sorry, I don't understand. What is encountered before freeing? nfc_llcp_socket_release() setting of socket state to closed. > > Thus, add a rwlock in struct llcp_sock to synchronize access to > > nfc_dev. nfc_dev in an llcp_sock will be NULLed in a write critical > > section when socket state has been set to closed. Thus, we can avoid > > the UAF by bailing out from a read critical section upon seeing NULL. > > > > [...] > > > > @@ -102,6 +102,7 @@ struct nfc_llcp_local { > > struct nfc_llcp_sock { > > struct sock sk; > > struct nfc_dev *dev; > > + rwlock_t rw_dev_lock; > > I dislike the idea of introducing the third (!!!) lock here. It looks > like a bandaid for this one particular problem. Yes, I see it now. Sorry about that. > > + pr_err("NFC device does not exit\n"); > > exist? Ouch, yes. I'll send a v2 improving the things. Thanks, Siddh
diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h index d8345ed57c95..800cbe8e3d6b 100644 --- a/net/nfc/llcp.h +++ b/net/nfc/llcp.h @@ -102,6 +102,7 @@ struct nfc_llcp_local { struct nfc_llcp_sock { struct sock sk; struct nfc_dev *dev; + rwlock_t rw_dev_lock; struct nfc_llcp_local *local; u32 target_idx; u32 nfc_protocol; diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c index 39c7c59bbf66..b132830bc206 100644 --- a/net/nfc/llcp_commands.c +++ b/net/nfc/llcp_commands.c @@ -315,13 +315,24 @@ static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock, { struct sk_buff *skb; int err, headroom, tailroom; + unsigned long irq_flags; if (sock->ssap == 0) return NULL; + read_lock_irqsave(&sock->rw_dev_lock, irq_flags); + + if (!sock->dev) { + read_unlock_irqrestore(&sock->rw_dev_lock, irq_flags); + pr_err("NFC device does not exit\n"); + return NULL; + } + headroom = sock->dev->tx_headroom; tailroom = sock->dev->tx_tailroom; + read_unlock_irqrestore(&sock->rw_dev_lock, irq_flags); + skb = nfc_alloc_send_skb(&sock->sk, MSG_DONTWAIT, size + LLCP_HEADER_SIZE, headroom, tailroom, &err); @@ -739,6 +750,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap, u8 *msg_ptr, *msg_data; u16 remote_miu; int err, headroom, tailroom; + unsigned long irq_flags; pr_debug("Send UI frame len %zd\n", len); @@ -746,6 +758,18 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap, if (local == NULL) return -ENODEV; + read_lock_irqsave(&sock->rw_dev_lock, irq_flags); + + if (!sock->dev) { + read_unlock_irqrestore(&sock->rw_dev_lock, irq_flags); + return -ENODEV; + } + + headroom = sock->dev->tx_headroom; + tailroom = sock->dev->tx_tailroom; + + read_unlock_irqrestore(&sock->rw_dev_lock, irq_flags); + msg_data = kmalloc(len, GFP_USER | __GFP_NOWARN); if (msg_data == NULL) return -ENOMEM; @@ -755,9 +779,6 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap, return -EFAULT; } - headroom = sock->dev->tx_headroom; - tailroom = sock->dev->tx_tailroom; - remaining_len = len; msg_ptr = msg_data; diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..a565712d7db8 100644 --- a/net/nfc/llcp_core.c +++ b/net/nfc/llcp_core.c @@ -20,6 +20,22 @@ static LIST_HEAD(llcp_devices); /* Protects llcp_devices list */ static DEFINE_SPINLOCK(llcp_devices_lock); +static inline void nfc_llcp_sock_close(struct nfc_llcp_sock *llcp_sock, int err) +{ + struct sock *sk = &llcp_sock->sk; + unsigned long irq_flags; + + if (err) + sk->sk_err = err; + + sk->sk_state = LLCP_CLOSED; + sk->sk_state_change(sk); + + write_lock_irqsave(&llcp_sock->rw_dev_lock, irq_flags); + llcp_sock->dev = NULL; + write_unlock_irqrestore(&llcp_sock->rw_dev_lock, irq_flags); +} + static void nfc_llcp_rx_skb(struct nfc_llcp_local *local, struct sk_buff *skb); void nfc_llcp_sock_link(struct llcp_sock_list *l, struct sock *sk) @@ -96,19 +112,13 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device, nfc_llcp_accept_unlink(accept_sk); - if (err) - accept_sk->sk_err = err; - accept_sk->sk_state = LLCP_CLOSED; - accept_sk->sk_state_change(sk); + nfc_llcp_sock_close(lsk, err); bh_unlock_sock(accept_sk); } } - if (err) - sk->sk_err = err; - sk->sk_state = LLCP_CLOSED; - sk->sk_state_change(sk); + nfc_llcp_sock_close(llcp_sock, err); bh_unlock_sock(sk); @@ -130,10 +140,7 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device, nfc_llcp_socket_purge(llcp_sock); - if (err) - sk->sk_err = err; - sk->sk_state = LLCP_CLOSED; - sk->sk_state_change(sk); + nfc_llcp_sock_close(llcp_sock, err); bh_unlock_sock(sk); diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c index 645677f84dba..ef1ab88a5e4f 100644 --- a/net/nfc/llcp_sock.c +++ b/net/nfc/llcp_sock.c @@ -983,6 +983,8 @@ struct sock *nfc_llcp_sock_alloc(struct socket *sock, int type, gfp_t gfp, int k sk->sk_type = type; sk->sk_destruct = llcp_sock_destruct; + rwlock_init(&llcp_sock->rw_dev_lock); + llcp_sock->ssap = 0; llcp_sock->dsap = LLCP_SAP_SDP; llcp_sock->rw = LLCP_MAX_RW + 1;
llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame(), which accesses the nfc_dev from the llcp_sock for getting the headroom and tailroom needed for skb allocation. Parallely, the nfc_dev can be freed via the nfc_unregister_device() codepath (nfc_release() being called due to the class unregister in nfc_exit()), leading to the UAF reported by Syzkaller. We have the following call tree before freeing: nfc_unregister_device() -> nfc_llcp_unregister_device() -> local_cleanup() -> nfc_llcp_socket_release() nfc_llcp_socket_release() sets the state of sockets to LLCP_CLOSED, and this is encountered necessarily before any freeing of nfc_dev. Thus, add a rwlock in struct llcp_sock to synchronize access to nfc_dev. nfc_dev in an llcp_sock will be NULLed in a write critical section when socket state has been set to closed. Thus, we can avoid the UAF by bailing out from a read critical section upon seeing NULL. Since this is repeated multiple times in nfc_llcp_socket_release(), extract the behaviour into a new function. Reported-and-tested-by: syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d Signed-off-by: Siddh Raman Pant <code@siddh.me> --- net/nfc/llcp.h | 1 + net/nfc/llcp_commands.c | 27 ++++++++++++++++++++++++--- net/nfc/llcp_core.c | 31 +++++++++++++++++++------------ net/nfc/llcp_sock.c | 2 ++ 4 files changed, 46 insertions(+), 15 deletions(-)