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 <code@siddh.me>
---
 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

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 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