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

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

[Jordy Zomer]

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 <jordy@pwning.systems>
---
 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

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

[Jakub Kicinski]

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!

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

[Jordy Zomer]

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 <jordy@pwning.systems>
---
 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

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

[Krzysztof Kozlowski]

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

> 
> Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> ---
>  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;

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.

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

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

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

[Jordy Zomer]

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 <jordy@pwning.systems>
---
 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

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

[Krzysztof Kozlowski]

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 <jordy@pwning.systems>
> ---
>  drivers/nfc/st-nci/se.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Looks ok.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

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

[Jakub Kicinski]

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 <jordy@pwning.systems>

This patch with more context:

> 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
> @@ -333,18 +333,28 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>                 transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);

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;