[PATCH] nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[PATCH] nfc: st21nfca: 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/st21nfca/se.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index a43fc4117fa5..e3483aca3280 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -316,6 +316,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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);
 
@@ -325,6 +330,14 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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

[PATCH v2] nfc: st21nfca: 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 the comment styles and removed double newlines

Signed-off-by: Jordy Zomer <jordy@pwning.systems>
---
 drivers/nfc/st21nfca/se.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index a43fc4117fa5..5e036768b2a1 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -316,6 +316,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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);
 
@@ -325,6 +330,16 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Krzysztof Kozlowski]

On 18/11/2021 08:04, 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 :)
> 
> EDIT: Changed the comment styles and removed double newlines
> 

Same comments apply. :)


Best regards,
Krzysztof

[PATCH v3] nfc: st21nfca: 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/st21nfca/se.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index a43fc4117fa5..c922f10d0d7b 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -316,6 +316,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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);
 
@@ -325,6 +330,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Krzysztof Kozlowski]

On 11/01/2022 17:44, 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/st21nfca/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: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Jakub Kicinski]

On Wed, 12 Jan 2022 11:07:34 +0100 Krzysztof Kozlowski wrote:
> On 11/01/2022 17:44, 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>
> 
> Looks ok.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Thanks! I believe this is commit 4fbcc1a4cb20 ("nfc: st21nfca: Fix
potential buffer overflows in EVT_TRANSACTION") in net.

[PATCH] nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Denis Efremov]

From: Jordy Zomer <jordy@pwning.systems>

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.

Link: https://lore.kernel.org/all/20220111164451.3232987-1-jordy@pwning.systems/#t
Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Cc: stable@vger.kernel.org # 4.0+
Signed-off-by: Jordy Zomer <jordy@pwning.systems>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Denis Efremov <denis.e.efremov@oracle.com>
---
CVE-2022-26490 was assigned to this patch. It looks like it's not in
the stable trees yet. I only added Link:/Fixes:/Cc: stable... to the
commit's message.

 drivers/nfc/st21nfca/se.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index a43fc4117fa5..c922f10d0d7b 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -316,6 +316,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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);
 
@@ -325,6 +330,11 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 			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.35.1

Re: [PATCH] nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Greg KH]

On Mon, Mar 21, 2022 at 08:40:06PM +0300, Denis Efremov wrote:
> From: Jordy Zomer <jordy@pwning.systems>
> 
> 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.
> 
> Link: https://lore.kernel.org/all/20220111164451.3232987-1-jordy@pwning.systems/#t
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Cc: stable@vger.kernel.org # 4.0+
> Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Denis Efremov <denis.e.efremov@oracle.com>
> ---
> CVE-2022-26490 was assigned to this patch. It looks like it's not in
> the stable trees yet. I only added Link:/Fixes:/Cc: stable... to the
> commit's message.
> 
>  drivers/nfc/st21nfca/se.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

What is the git commit id of this patch in Linus's tree?

And your "To:" line was corrupted, might want to check your email client :(

thanks,

greg k-h

Re: [External] : Re: [PATCH] nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Denis Efremov]

On 3/22/22 10:11, Greg KH wrote:
> On Mon, Mar 21, 2022 at 08:40:06PM +0300, Denis Efremov wrote:

>> CVE-2022-26490 was assigned to this patch. It looks like it's not in
>> the stable trees yet. I only added Link:/Fixes:/Cc: stable... to the
>> commit's message.
>>
>>  drivers/nfc/st21nfca/se.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
> 
> What is the git commit id of this patch in Linus's tree?
> 

commit 4fbcc1a4cb20fe26ad0225679c536c80f1648221 upstream.

Thanks,
Denis

Re: [External] : Re: [PATCH] nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION

[Greg KH]

On Tue, Mar 22, 2022 at 11:58:04AM +0300, Denis Efremov wrote:
> 
> 
> On 3/22/22 10:11, Greg KH wrote:
> > On Mon, Mar 21, 2022 at 08:40:06PM +0300, Denis Efremov wrote:
> 
> >> CVE-2022-26490 was assigned to this patch. It looks like it's not in
> >> the stable trees yet. I only added Link:/Fixes:/Cc: stable... to the
> >> commit's message.
> >>
> >>  drivers/nfc/st21nfca/se.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> > 
> > What is the git commit id of this patch in Linus's tree?
> > 
> 
> commit 4fbcc1a4cb20fe26ad0225679c536c80f1648221 upstream.

Now queued up, thanks.

greg k-h