(This patchset should be applied in order as changes are dependent.)
For connectionless transmission, llcp_sock_sendmsg() codepath will
eventually call nfc_alloc_send_skb() which takes in an nfc_dev as
an argument for calculating the total size for skb allocation.
virtual_ncidev_close() codepath eventually releases socket by calling
nfc_llcp_socket_release() (which sets the sk->sk_state to LLCP_CLOSED)
and afterwards the nfc_dev will be eventually freed.
When an ndev gets freed, llcp_sock_sendmsg() will result in an
use-after-free as it
(1) doesn't have any checks in place for avoiding the datagram sending.
(2) calls nfc_llcp_send_ui_frame(), which also has a do-while loop which
can race with freeing (a msg with size of 4096 is sent in chunks of
128 in this repro). This loop contains the call to nfc_alloc_send_skb().
Thus, I extracted nfc_dev access from nfc_alloc_send_skb() to the caller,
and used a spinlock (rwlock) to protect/serialize access to the nfc_dev to
avoid the UAF. Tested with syzkaller.
Since state has to be LLCP_BOUND for datagram sending, we can bail out early
in llcp_sock_sendmsg().
The last patch is just a code reformat. As the datagram sending is a bigger
chunk of the code, we can reduce unnecessary indentation.
Please review and let me know if any errors are there, and hopefully this
gets accepted.
Thanks,
Siddh
Siddh Raman Pant (4):
nfc: Extract nfc_dev access from nfc_alloc_send_skb() into the callers
nfc: Protect access to nfc_dev in an llcp_sock with a rwlock
nfc: Do not send datagram if socket state isn't LLCP_BOUND
nfc: llcp_sock_sendmsg: Reformat code to make the smaller block
indented
include/net/nfc/nfc.h | 6 +++---
net/nfc/core.c | 14 +++++++-------
net/nfc/llcp.h | 1 +
net/nfc/llcp_commands.c | 41 +++++++++++++++++++++++++++++++++++------
net/nfc/llcp_core.c | 31 +++++++++++++++++++------------
net/nfc/llcp_sock.c | 31 ++++++++++++++++++-------------
net/nfc/rawsock.c | 8 ++++++--
7 files changed, 89 insertions(+), 43 deletions(-)
--
2.42.0
The only reason why nfc_dev was accessed inside nfc_alloc_send_skb() is
for getting the headroom and tailroom values.
This can cause UAF to be reported from nfc_alloc_send_skb(), but the
callers are responsible for managing the device access, and thus the
UAF being reported, as the callers (like nfc_llcp_send_ui_frame()) may
repeatedly call this function, and this function will repeatedly try
to get the same headroom and tailroom values.
Thus, put the nfc_dev access responsibility on the callers and accept
the headroom and tailroom values directly.
Signed-off-by: Siddh Raman Pant <[email protected]>
---
include/net/nfc/nfc.h | 6 +++---
net/nfc/core.c | 14 +++++++-------
net/nfc/llcp_commands.c | 20 ++++++++++++++------
net/nfc/rawsock.c | 8 ++++++--
4 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
index 5dee575fbe86..efe20a9a8499 100644
--- a/include/net/nfc/nfc.h
+++ b/include/net/nfc/nfc.h
@@ -260,9 +260,9 @@ static inline const char *nfc_device_name(const struct nfc_dev *dev)
return dev_name(&dev->dev);
}
-struct sk_buff *nfc_alloc_send_skb(struct nfc_dev *dev, struct sock *sk,
- unsigned int flags, unsigned int size,
- unsigned int *err);
+struct sk_buff *nfc_alloc_send_skb(struct sock *sk, unsigned int flags,
+ unsigned int size, int headroom,
+ int tailroom, unsigned int *err);
struct sk_buff *nfc_alloc_recv_skb(unsigned int size, gfp_t gfp);
int nfc_set_remote_general_bytes(struct nfc_dev *dev,
diff --git a/net/nfc/core.c b/net/nfc/core.c
index eb2c0959e5b6..1f7d20971f6f 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -705,25 +705,25 @@ EXPORT_SYMBOL(nfc_tm_deactivated);
/**
* nfc_alloc_send_skb - allocate a skb for data exchange responses
*
- * @dev: device sending the response
* @sk: socket sending the response
* @flags: MSG_DONTWAIT flag
* @size: size to allocate
+ * @headroom: Extra headroom, in addition to size
+ * @tailroom: Extra tailroom, in addition to size
* @err: pointer to memory to store the error code
*/
-struct sk_buff *nfc_alloc_send_skb(struct nfc_dev *dev, struct sock *sk,
- unsigned int flags, unsigned int size,
- unsigned int *err)
+struct sk_buff *nfc_alloc_send_skb(struct sock *sk, unsigned int flags,
+ unsigned int size, int headroom,
+ int tailroom, unsigned int *err)
{
struct sk_buff *skb;
unsigned int total_size;
- total_size = size +
- dev->tx_headroom + dev->tx_tailroom + NFC_HEADER_SIZE;
+ total_size = size + headroom + tailroom + NFC_HEADER_SIZE;
skb = sock_alloc_send_skb(sk, total_size, flags & MSG_DONTWAIT, err);
if (skb)
- skb_reserve(skb, dev->tx_headroom + NFC_HEADER_SIZE);
+ skb_reserve(skb, headroom + NFC_HEADER_SIZE);
return skb;
}
diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c
index e2680a3bef79..39c7c59bbf66 100644
--- a/net/nfc/llcp_commands.c
+++ b/net/nfc/llcp_commands.c
@@ -314,13 +314,17 @@ static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock,
u8 cmd, u16 size)
{
struct sk_buff *skb;
- int err;
+ int err, headroom, tailroom;
if (sock->ssap == 0)
return NULL;
- skb = nfc_alloc_send_skb(sock->dev, &sock->sk, MSG_DONTWAIT,
- size + LLCP_HEADER_SIZE, &err);
+ headroom = sock->dev->tx_headroom;
+ tailroom = sock->dev->tx_tailroom;
+
+ skb = nfc_alloc_send_skb(&sock->sk, MSG_DONTWAIT,
+ size + LLCP_HEADER_SIZE, headroom, tailroom,
+ &err);
if (skb == NULL) {
pr_err("Could not allocate PDU\n");
return NULL;
@@ -734,7 +738,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap,
size_t frag_len = 0, remaining_len;
u8 *msg_ptr, *msg_data;
u16 remote_miu;
- int err;
+ int err, headroom, tailroom;
pr_debug("Send UI frame len %zd\n", len);
@@ -751,6 +755,9 @@ 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;
@@ -763,8 +770,9 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap,
pr_debug("Fragment %zd bytes remaining %zd",
frag_len, remaining_len);
- pdu = nfc_alloc_send_skb(sock->dev, &sock->sk, 0,
- frag_len + LLCP_HEADER_SIZE, &err);
+ pdu = nfc_alloc_send_skb(&sock->sk, 0,
+ frag_len + LLCP_HEADER_SIZE,
+ headroom, tailroom, &err);
if (pdu == NULL) {
pr_err("Could not allocate PDU (error=%d)\n", err);
len -= remaining_len;
diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c
index 5125392bb68e..fab1facb7105 100644
--- a/net/nfc/rawsock.c
+++ b/net/nfc/rawsock.c
@@ -207,7 +207,7 @@ static int rawsock_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
struct sock *sk = sock->sk;
struct nfc_dev *dev = nfc_rawsock(sk)->dev;
struct sk_buff *skb;
- int rc;
+ int rc, headroom, tailroom;
pr_debug("sock=%p sk=%p len=%zu\n", sock, sk, len);
@@ -217,7 +217,11 @@ static int rawsock_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
if (sock->state != SS_CONNECTED)
return -ENOTCONN;
- skb = nfc_alloc_send_skb(dev, sk, msg->msg_flags, len, &rc);
+ headroom = dev->tx_headroom;
+ tailroom = dev->tx_tailroom;
+
+ skb = nfc_alloc_send_skb(sk, msg->msg_flags, len, headroom, tailroom,
+ &rc);
if (skb == NULL)
return rc;
--
2.42.0
As we know we cannot send the datagram (state can be set to LLCP_CLOSED
by nfc_llcp_socket_release()), there is no need to proceed further.
Thus, bail out early from llcp_sock_sendmsg().
Signed-off-by: Siddh Raman Pant <[email protected]>
---
net/nfc/llcp_sock.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index ef1ab88a5e4f..603f2219b62f 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -796,6 +796,11 @@ static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg,
}
if (sk->sk_type == SOCK_DGRAM) {
+ if (sk->sk_state != LLCP_BOUND) {
+ release_sock(sk);
+ return -ENOTCONN;
+ }
+
DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr,
msg->msg_name);
--
2.42.0
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: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d
Signed-off-by: Siddh Raman Pant <[email protected]>
---
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;
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;
--
2.42.0
The block for datagram sending is a significantly bigger chunk of the
function compared to the other scenario.
Thus, put the significantly smaller block inside the if-block.
Signed-off-by: Siddh Raman Pant <[email protected]>
---
net/nfc/llcp_sock.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 603f2219b62f..3f1a39e54aa1 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -795,34 +795,32 @@ static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg,
return -ENODEV;
}
- if (sk->sk_type == SOCK_DGRAM) {
- if (sk->sk_state != LLCP_BOUND) {
- release_sock(sk);
- return -ENOTCONN;
- }
+ if (sk->sk_type != SOCK_DGRAM) {
+ release_sock(sk);
- DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr,
- msg->msg_name);
+ if (sk->sk_state != LLCP_CONNECTED)
+ return -ENOTCONN;
- if (msg->msg_namelen < sizeof(*addr)) {
- release_sock(sk);
- return -EINVAL;
- }
+ return nfc_llcp_send_i_frame(llcp_sock, msg, len);
+ }
+ if (sk->sk_state != LLCP_BOUND) {
release_sock(sk);
-
- return nfc_llcp_send_ui_frame(llcp_sock, addr->dsap, addr->ssap,
- msg, len);
+ return -ENOTCONN;
}
- if (sk->sk_state != LLCP_CONNECTED) {
+ DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr, msg->msg_name);
+
+ if (msg->msg_namelen < sizeof(*addr)) {
release_sock(sk);
- return -ENOTCONN;
+ return -EINVAL;
}
release_sock(sk);
- return nfc_llcp_send_i_frame(llcp_sock, msg, len);
+ return nfc_llcp_send_ui_frame(llcp_sock, addr->dsap, addr->ssap,
+ msg, len);
+
}
static int llcp_sock_recvmsg(struct socket *sock, struct msghdr *msg,
--
2.42.0
On 25/11/2023 21:26, Siddh Raman Pant wrote:
> (This patchset should be applied in order as changes are dependent.)
>
Fixes should be independent of reformat and refactorings. Please add
necessary Fixes tags and decouple the non-fixes part of patchset.
Also, don't forget about net-next patch subject.
Best regards,
Krzysztof
On 25/11/2023 21:26, Siddh Raman Pant wrote:
> The only reason why nfc_dev was accessed inside nfc_alloc_send_skb() is
> for getting the headroom and tailroom values.
>
> This can cause UAF to be reported from nfc_alloc_send_skb(), but the
> callers are responsible for managing the device access, and thus the
> UAF being reported, as the callers (like nfc_llcp_send_ui_frame()) may
> repeatedly call this function, and this function will repeatedly try
> to get the same headroom and tailroom values.
I don't understand this sentence.
"This can cause ..., but ...". But starts another clause which should be
in contradictory to previous one.
>
> Thus, put the nfc_dev access responsibility on the callers and accept
> the headroom and tailroom values directly.
Is this a fix or improvement? If fix, is the UAF real? If so, you miss
Fixes tag.
Best regards,
Krzysztof
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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d
> Signed-off-by: Siddh Raman Pant <[email protected]>
> ---
> 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 25/11/2023 21:26, Siddh Raman Pant wrote:
> As we know we cannot send the datagram (state can be set to LLCP_CLOSED
> by nfc_llcp_socket_release()), there is no need to proceed further.
>
> Thus, bail out early from llcp_sock_sendmsg().
>
> Signed-off-by: Siddh Raman Pant <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 25/11/2023 21:26, Siddh Raman Pant wrote:
> The block for datagram sending is a significantly bigger chunk of the
> function compared to the other scenario.
>
> Thus, put the significantly smaller block inside the if-block.
>
>
> + if (sk->sk_state != LLCP_BOUND) {
> release_sock(sk);
> -
> - return nfc_llcp_send_ui_frame(llcp_sock, addr->dsap, addr->ssap,
> - msg, len);
> + return -ENOTCONN;
> }
>
> - if (sk->sk_state != LLCP_CONNECTED) {
> + DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, addr, msg->msg_name);
No, this code is not readable. I don't think this change helps in anything.
> +
> + if (msg->msg_namelen < sizeof(*addr)) {
> release_sock(sk);
> - return -ENOTCONN;
> + return -EINVAL;
> }
>
> release_sock(sk);
>
> - return nfc_llcp_send_i_frame(llcp_sock, msg, len);
> + return nfc_llcp_send_ui_frame(llcp_sock, addr->dsap, addr->ssap,
> + msg, len);
> +
Stray blank line.
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
On Mon, 27 Nov 2023 15:40:51 +0530, Krzysztof Kozlowski wrote:
> On 25/11/2023 21:26, Siddh Raman Pant wrote:
> > The only reason why nfc_dev was accessed inside nfc_alloc_send_skb() is
> > for getting the headroom and tailroom values.
> >
> > This can cause UAF to be reported from nfc_alloc_send_skb(), but the
> > callers are responsible for managing the device access, and thus the
> > UAF being reported, as the callers (like nfc_llcp_send_ui_frame()) may
> > repeatedly call this function, and this function will repeatedly try
> > to get the same headroom and tailroom values.
>
> I don't understand this sentence.
>
> "This can cause ..., but ...". But starts another clause which should be
> in contradictory to previous one.
Sorry about that, I should have phrased it better.
> > Thus, put the nfc_dev access responsibility on the callers and accept
> > the headroom and tailroom values directly.
>
> Is this a fix or improvement? If fix, is the UAF real? If so, you miss
> Fixes tag.
I intended to remove access to nfc_dev (accessing which causes UAF) inside
this function, as it is used only for fetching headroom and tailroom integral
values.
nfc_llcp_send_ui_frame() called this function in a do-while loop, so
I thought of extracting the values before the loop, so that in the next
patch where I used locking, I would have to lock only once*.
Since these are two units of changes, I separated them into two patches.
Though since the next patch is shit anyways, this patch is not needed.
Thanks,
Siddh