Skip to contentSkip to conversation

Fix UAF caused by racing datagram sending and freeing of nfc_dev

PATCH In review Siddh Raman Pant 2 participants 12 messages Nov 25, 2023 – Dec 2, 2023

Subsystems (3)
Networking Network drivers NFC
lore
Share & tools
lore

Siddh Raman Pant

author
#0 lore raw

cover Fix UAF caused by racing datagram sending and freeing of nfc_dev

(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(-)
Signature · 1 line
2.42.0
author
#1 lore raw

PATCH 1/4 nfc: Extract nfc_dev access from nfc_alloc_send_skb() into the callers

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(-)
include/net/nfc/nfc.h +3 -3
index 5dee575fbe86..efe20a9a8499 100644
@@ -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,
net/nfc/core.c +7 -7
index eb2c0959e5b6..1f7d20971f6f 100644
@@ -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;
 }
net/nfc/llcp_commands.c +14 -6
index e2680a3bef79..39c7c59bbf66 100644
@@ -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;
net/nfc/rawsock.c +6 -2
index 5125392bb68e..fab1facb7105 100644
@@ -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;
Signature · 1 line
2.42.0
#2 lore raw

Re: [PATCH 1/4] nfc: Extract nfc_dev access from nfc_alloc_send_skb() into the callers

Quoted 8 lines · Siddh Raman Pant

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.

Quoted 3 lines

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

author
#3 lore raw

Re: [PATCH 1/4] nfc: Extract nfc_dev access from nfc_alloc_send_skb() into the callers

Quoted 10 lines · Krzysztof Kozlowski
Quoted 8 lines · Siddh Raman Pant

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.

Quoted 4 lines

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.

Quoted 5 lines
Quoted 2 lines

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

author
#4 lore raw

PATCH 2/4 nfc: Protect access to nfc_dev in an llcp_sock with a rwlock

In reply to Siddh Raman Pant

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(-)
net/nfc/llcp.h +1 -0
index d8345ed57c95..800cbe8e3d6b 100644
@@ -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;
net/nfc/llcp_commands.c +24 -3
index 39c7c59bbf66..b132830bc206 100644
@@ -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;
net/nfc/llcp_core.c +19 -12
index 1dac28136e6a..a565712d7db8 100644
@@ -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);
net/nfc/llcp_sock.c +2 -0
index 645677f84dba..ef1ab88a5e4f 100644
@@ -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;
Signature · 1 line
2.42.0
#5 lore raw

Re: [PATCH 2/4] nfc: Protect access to nfc_dev in an llcp_sock with a rwlock

Quoted 3 lines · Siddh Raman Pant

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?

Quoted 7 lines

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:

Quoted 7 lines

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?

Quoted 24 lines

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(-)
net/nfc/llcp.h +0 -0
index d8345ed57c95..800cbe8e3d6b 100644

net/nfc/llcp.h struct nfc_llcp_local { · line 105

Quoted 4 lines
net/nfc/llcp.h +1 -0
@@ -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.

Quoted 11 lines
net/nfc/llcp.h +0 -0
 	struct nfc_llcp_local *local;
 	u32 target_idx;
 	u32 nfc_protocol;
net/nfc/llcp_commands.c +0 -0
index 39c7c59bbf66..b132830bc206 100644
@@ -315,13 +315,24 @@ static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock,
 {
 	struct sk_buff *skb;
 	int err, headroom, tailroom;

net/nfc/llcp_commands.c static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock, · line 327

Quoted 10 lines
net/nfc/llcp_commands.c +6 -0
+	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

author
#6 lore raw

Re: [PATCH 2/4] nfc: Protect access to nfc_dev in an llcp_sock with a rwlock

Quoted 7 lines · Krzysztof Kozlowski
Quoted 3 lines · Siddh Raman Pant

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.

Quoted 11 lines
Quoted 11 lines

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()
Quoted 4 lines
Quoted 2 lines

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.

Quoted 7 lines
Quoted 7 lines

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.

[...]

Quoted 8 lines

net/nfc/llcp.h struct nfc_llcp_local { · line 105

Quoted 5 lines
net/nfc/llcp.h +1 -0
@@ -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.

Quoted 3 lines

net/nfc/llcp_commands.c static struct sk_buff *llcp_allocate_pdu(struct nfc_llcp_sock *sock, · line 327

Quoted 1 line
net/nfc/llcp_commands.c +1 -0
+		pr_err("NFC device does not exit\n");

exist?

Ouch, yes.

I'll send a v2 improving the things.

Thanks,
Siddh

author
#7 lore raw

PATCH 3/4 nfc: Do not send datagram if socket state isn't LLCP_BOUND

In reply to Siddh Raman Pant

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(+)
net/nfc/llcp_sock.c +5 -0
index ef1ab88a5e4f..603f2219b62f 100644
@@ -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);
Signature · 1 line
2.42.0
author
#9 lore raw

PATCH 4/4 nfc: llcp_sock_sendmsg: Reformat code to make the smaller block indented

In reply to Siddh Raman Pant

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(-)
net/nfc/llcp_sock.c +15 -17
index 603f2219b62f..3f1a39e54aa1 100644
@@ -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,
Signature · 1 line
2.42.0
#10 lore raw

Re: [PATCH 4/4] nfc: llcp_sock_sendmsg: Reformat code to make the smaller block indented

Quoted 8 lines · Siddh Raman Pant

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.

net/nfc/llcp_sock.c static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg, · line 812

Quoted 10 lines
net/nfc/llcp_sock.c +3 -4
+	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.

net/nfc/llcp_sock.c static int llcp_sock_sendmsg(struct socket *sock, struct msghdr *msg, · line 823

Quoted 13 lines
net/nfc/llcp_sock.c +6 -2
+
+	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

#11 lore raw
In reply to Siddh Raman Pant
Quoted 2 lines · Siddh Raman Pant

(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