[PATCH] nfc: st-nci: Fix potential buffer overflows in EVT_TRANSACTION

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

It would be nice if someone can review and test this patch because
I don't own the hardware :)

Signed-off-by: Jordy Zomer <redacted>
---
 drivers/nfc/st-nci/se.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 7764b1a4c3cf..0d22853925d8 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c

@@ -335,6 +335,12 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -ENOMEM;
 
 		transaction->aid_len = skb->data[1];
+
+		// Checking if the length of the AID is valid
+		if (transaction->aid_len > sizeof(transaction->aid))
+			return -EINVAL;
+
+
 		memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
 
 		/* Check next byte is PARAMETERS tag (82) */

@@ -343,6 +349,15 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -EPROTO;
 
 		transaction->params_len = skb->data[transaction->aid_len + 3];
+
+		// check if the length of the parameters is valid
+		// we can't use sizeof(transaction->params) because it's
+		// a flexible array member so we have to check if params_len
+		// is bigger than the space allocated for the array
+		if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
+			return -EINVAL;
+
+
 		memcpy(transaction->params, skb->data +
 		       transaction->aid_len + 4, transaction->params_len);
 

-- 
2.27.0

On Wed, 17 Nov 2021 18:15:51 +0100 Jordy Zomer wrote:

+
+		// Checking if the length of the AID is valid
+		if (transaction->aid_len > sizeof(transaction->aid))
+			return -EINVAL;
+
+

Please remove the double blank lines and use more common style of
multi-line comments /* */ like the rest of this file.

Same for the other patch. Thanks!

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

It would be nice if someone can review and test this patch because
I don't own the hardware :)

EDIT: Changed comment style and double newlines

Signed-off-by: Jordy Zomer <redacted>
---
 drivers/nfc/st-nci/se.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 7764b1a4c3cf..8e2ac8a3d199 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c

@@ -335,6 +335,11 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -ENOMEM;
 
 		transaction->aid_len = skb->data[1];
+
+		/* Checking if the length of the AID is valid */
+		if (transaction->aid_len > sizeof(transaction->aid))
+			return -EINVAL;
+
 		memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
 
 		/* Check next byte is PARAMETERS tag (82) */

@@ -343,6 +348,16 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -EPROTO;
 
 		transaction->params_len = skb->data[transaction->aid_len + 3];
+
+		/*
+		 * check if the length of the parameters is valid
+		 * we can't use sizeof(transaction->params) because it's
+		 * a flexible array member so we have to check if params_len
+		 * is bigger than the space allocated for the array
+		 */
+		if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
+			return -EINVAL;
+
 		memcpy(transaction->params, skb->data +
 		       transaction->aid_len + 4, transaction->params_len);
 

-- 
2.27.0

On 18/11/2021 08:02, Jordy Zomer wrote:

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

It would be nice if someone can review and test this patch because
I don't own the hardware :)

Thanks for your patch.
You mentioned that there are buffer overflows but you do not own the
hardware. How do you know these overflow exist? How did you detect them?

EDIT: Changed comment style and double newlines

Please add changelog after --- separators so it does not clutter the
commit log with unrelated "EDIT".

I am thinking whether the check should be before memory allocation - to
save on useless memory allocation in case of error, but make the code
less obvious with referring to skb->data[1] twice.

The current comment is long and actually not explaining how you get "-2"
and sizeof, so how about:
"Total size is allocated (skb->len - 2) minus fixed array members)"

In general the code looks ok, although I cannot provide tests.

+
 		memcpy(transaction->params, skb->data +
 		       transaction->aid_len + 4, transaction->params_len);
 

Best regards,
Krzysztof

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

Signed-off-by: Jordy Zomer <redacted>
---
 drivers/nfc/st-nci/se.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 7764b1a4c3cf..cdb59ddff4e8 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c

@@ -335,6 +335,11 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -ENOMEM;
 
 		transaction->aid_len = skb->data[1];
+
+		/* Checking if the length of the AID is valid */
+		if (transaction->aid_len > sizeof(transaction->aid))
+			return -EINVAL;
+
 		memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
 
 		/* Check next byte is PARAMETERS tag (82) */

@@ -343,6 +348,11 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 			return -EPROTO;
 
 		transaction->params_len = skb->data[transaction->aid_len + 3];
+
+		/* Total size is allocated (skb->len - 2) minus fixed array members */
+		if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
+			return -EINVAL;
+
 		memcpy(transaction->params, skb->data +
 		       transaction->aid_len + 4, transaction->params_len);
 

-- 
2.27.0

On 11/01/2022 17:45, Jordy Zomer wrote:

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

Signed-off-by: Jordy Zomer <redacted>
---
 drivers/nfc/st-nci/se.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Looks ok.

Reviewed-by: Krzysztof Kozlowski <redacted>


Best regards,
Krzysztof

On Tue, 11 Jan 2022 17:45:43 +0100 Jordy Zomer wrote:

It appears that there are some buffer overflows in EVT_TRANSACTION.
This happens because the length parameters that are passed to memcpy
come directly from skb->data and are not guarded in any way.

Signed-off-by: Jordy Zomer <redacted>

This patch with more context:

What checks skb->len > 2 ?

                if (!transaction)
                        return -ENOMEM;

Leaks skb ?

                transaction->aid_len = skb->data[1];
+
+               /* Checking if the length of the AID is valid */
+               if (transaction->aid_len > sizeof(transaction->aid))
+                       return -EINVAL;

                memcpy(transaction->aid, &skb->data[2], transaction->aid_len);

What checks skb->len > 2 + transaction->aid_len ?

                /* Check next byte is PARAMETERS tag (82) */
                if (skb->data[transaction->aid_len + 2] !=

.. make that skb->len > 2 + transaction->aid_len + 1

                    NFC_EVT_TRANSACTION_PARAMS_TAG)
                        return -EPROTO;

Leaks skb ? (btw devm_kmalloc() in message processing could probably as well be counted 
as leak unless something guarantees attacker can't generate infinite messages of this type)

                transaction->params_len = skb->data[transaction->aid_len + 3];

.. skb->len > 2 + transaction->aid_len + 1 + 1

+               /* Total size is allocated (skb->len - 2) minus fixed array members */
+               if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))

So this check makes sure we don't overflow transaction->params, right?
Again, does skb->len not have to be validated as well?

+                       return -EINVAL;
+
                memcpy(transaction->params, skb->data +
                       transaction->aid_len + 4, transaction->params_len);
 
                r = nfc_se_transaction(ndev->nfc_dev, host, transaction);
                break;