[PATCH net v3 0/3] Split "nfc: st21nfca: Refactor EVT_TRANSACTION" into 3

[PATCH net v3 0/3] Split "nfc: st21nfca: Refactor EVT_TRANSACTION" into 3

[Martin Faltesek]

Change log:

v2 -> v3:

1. v2 review comment: modified sender email to match SoB line.

2. v2 review comment: threading emails by using git send-email as
   recommended.

3. v2 review comment: added linux-nfc@lists.01.org. Tried to join
   list but no reply so not sure if this messages makes it through.

4. v1 review comment: use net style multi-line comments.
   This affected two multi-line comments in:
   1/3
   nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

5. added Cc: stable@vger.kernel.org in signoff area of each patch.

v1 -> v2:

   Split the original patch into 3 patches, so that each one solves
   a single issue. The original patch indicated 4 bugs, but two are
   so closely related that I feel it makes sense to keep them
   together.

   1/3
   nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION

   This is mentioned in v1 as #1.  It just changes logical AND to
   logical OR. The AND was rendering the check useless.

   2/3
   nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling

   This is from v1 #3.

   3/3
   nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

   This is from v1 #2 and #4
   Both are derived from the same bug, which is the incorrect calculation
   that buffer allocation size is skb->len - 2, so both should be combined.

   After these 3 patches are applied, the end result is the same as v1
   except:

   1. minor comment rewording.
   2. removed comments which felt superfluous explanations of obvious code.


v2: https://lore.kernel.org/netdev/20220401180939.2025819-1-mfaltesek@google.com/

v1: https://lore.kernel.org/netdev/20220329175431.3175472-1-mfaltesek@google.com/

Martin Faltesek (3):
  nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION
  nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling
  nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

 drivers/nfc/st21nfca/se.c | 53 ++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 23 deletions(-)


base-commit: b8d91399775c55162073bb2aca061ec42e3d4bc1
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net v3 1/3] nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION

[Martin Faltesek]

The first validation check for EVT_TRANSACTION has two different checks
tied together with logical AND. One is a check for minimum packet length,
and the other is for a valid aid_tag. If either condition is true (fails),
then an error should be triggered.  The fix is to change && to ||.

Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st21nfca/se.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index 7e213f8ddc98..9645777f2544 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -315,7 +315,7 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 		 * AID		81	5 to 16
 		 * PARAMETERS	82	0 to 255
 		 */
-		if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
+		if (skb->len < NFC_MIN_AID_LENGTH + 2 ||
 		    skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
 			return -EPROTO;
 
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH net v3 1/3] nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION

[Guenter Roeck]

On Mon, Jun 6, 2022 at 7:57 PM Martin Faltesek <mfaltesek@google.com> wrote:
>
> The first validation check for EVT_TRANSACTION has two different checks
> tied together with logical AND. One is a check for minimum packet length,
> and the other is for a valid aid_tag. If either condition is true (fails),
> then an error should be triggered.  The fix is to change && to ||.
>
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>  drivers/nfc/st21nfca/se.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index 7e213f8ddc98..9645777f2544 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -315,7 +315,7 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
>                  * AID          81      5 to 16
>                  * PARAMETERS   82      0 to 255
>                  */
> -               if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
> +               if (skb->len < NFC_MIN_AID_LENGTH + 2 ||
>                     skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
>                         return -EPROTO;
>
> --
> 2.36.1.255.ge46751e96f-goog
>

Re: [PATCH net v3 1/3] nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION

[Krzysztof Kozlowski]

On 07/06/2022 04:57, Martin Faltesek wrote:
> The first validation check for EVT_TRANSACTION has two different checks
> tied together with logical AND. One is a check for minimum packet length,
> and the other is for a valid aid_tag. If either condition is true (fails),
> then an error should be triggered.  The fix is to change && to ||.
> 
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>
> ---
>  drivers/nfc/st21nfca/se.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540

If a tag was not added on purpose, please state why and what changed.



Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH net v3 2/3] nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling

[Martin Faltesek]

Error paths do not free previously allocated memory. Add devm_kfree() to
those failure paths.

Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st21nfca/se.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index 9645777f2544..8e1113ce139b 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -326,22 +326,29 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 		transaction->aid_len = skb->data[1];
 
 		/* Checking if the length of the AID is valid */
-		if (transaction->aid_len > sizeof(transaction->aid))
+		if (transaction->aid_len > sizeof(transaction->aid)) {
+			devm_kfree(dev, transaction);
 			return -EINVAL;
+		}
 
 		memcpy(transaction->aid, &skb->data[2],
 		       transaction->aid_len);
 
 		/* Check next byte is PARAMETERS tag (82) */
 		if (skb->data[transaction->aid_len + 2] !=
-		    NFC_EVT_TRANSACTION_PARAMS_TAG)
+		    NFC_EVT_TRANSACTION_PARAMS_TAG) {
+			devm_kfree(dev, transaction);
 			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)))
+		if (transaction->params_len > ((skb->len - 2) -
+		    sizeof(struct nfc_evt_transaction))) {
+			devm_kfree(dev, transaction);
 			return -EINVAL;
+		}
 
 		memcpy(transaction->params, skb->data +
 		       transaction->aid_len + 4, transaction->params_len);
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH net v3 2/3] nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling

[Guenter Roeck]

On Mon, Jun 6, 2022 at 7:57 PM Martin Faltesek <mfaltesek@google.com> wrote:
>
> Error paths do not free previously allocated memory. Add devm_kfree() to
> those failure paths.
>
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>  drivers/nfc/st21nfca/se.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index 9645777f2544..8e1113ce139b 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -326,22 +326,29 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
>                 transaction->aid_len = skb->data[1];
>
>                 /* Checking if the length of the AID is valid */
> -               if (transaction->aid_len > sizeof(transaction->aid))
> +               if (transaction->aid_len > sizeof(transaction->aid)) {
> +                       devm_kfree(dev, transaction);
>                         return -EINVAL;
> +               }
>
>                 memcpy(transaction->aid, &skb->data[2],
>                        transaction->aid_len);
>
>                 /* Check next byte is PARAMETERS tag (82) */
>                 if (skb->data[transaction->aid_len + 2] !=
> -                   NFC_EVT_TRANSACTION_PARAMS_TAG)
> +                   NFC_EVT_TRANSACTION_PARAMS_TAG) {
> +                       devm_kfree(dev, transaction);
>                         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)))
> +               if (transaction->params_len > ((skb->len - 2) -
> +                   sizeof(struct nfc_evt_transaction))) {
> +                       devm_kfree(dev, transaction);
>                         return -EINVAL;
> +               }
>
>                 memcpy(transaction->params, skb->data +
>                        transaction->aid_len + 4, transaction->params_len);
> --
> 2.36.1.255.ge46751e96f-goog
>

Re: [PATCH net v3 2/3] nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling

[Krzysztof Kozlowski]

On 07/06/2022 04:57, Martin Faltesek wrote:
> Error paths do not free previously allocated memory. Add devm_kfree() to
> those failure paths.
> 
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Standard disclaimer:
---------
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540

If a tag was not added on purpose, please state why and what changed.
---------

So you dropped all my review tags?

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH net v3 3/3] nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

[Martin Faltesek]

The transaction buffer is allocated by using the size of the packet buf,
and subtracting two which seem intended to remove the two tags which are
not present in the target structure. This calculation leads to under
counting memory because of differences between the packet contents and the
target structure. The aid_len field is a u8 in the packet, but a u32 in
the structure, resulting in at least 3 bytes always being under counted.
Further, the aid data is a variable length field in the packet, but fixed
in the structure, so if this field is less than the max, the difference is
added to the under counting.

The last validation check for transaction->params_len is also incorrect
since it employs the same accounting error.

To fix, perform validation checks progressively to safely reach the
next field, to determine the size of both buffers and verify both tags.
Once all validation checks pass, allocate the buffer and copy the data.
This eliminates freeing memory on the error path, as those checks are
moved ahead of memory allocation.

Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st21nfca/se.c | 60 +++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index 8e1113ce139b..df8d27cf2956 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -300,6 +300,8 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 	int r = 0;
 	struct device *dev = &hdev->ndev->dev;
 	struct nfc_evt_transaction *transaction;
+	u32 aid_len;
+	u8 params_len;
 
 	pr_debug("connectivity gate event: %x\n", event);
 
@@ -308,50 +310,48 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
 		r = nfc_se_connectivity(hdev->ndev, host);
 	break;
 	case ST21NFCA_EVT_TRANSACTION:
-		/*
-		 * According to specification etsi 102 622
+		/* According to specification etsi 102 622
 		 * 11.2.2.4 EVT_TRANSACTION Table 52
 		 * Description	Tag	Length
 		 * AID		81	5 to 16
 		 * PARAMETERS	82	0 to 255
+		 *
+		 * The key differences are aid storage length is variably sized
+		 * in the packet, but fixed in nfc_evt_transaction, and that the aid_len
+		 * is u8 in the packet, but u32 in the structure, and the tags in
+		 * the packet are not included in nfc_evt_transaction.
+		 *
+		 * size in bytes: 1          1       5-16 1             1           0-255
+		 * offset:        0          1       2    aid_len + 2   aid_len + 3 aid_len + 4
+		 * member name:   aid_tag(M) aid_len aid  params_tag(M) params_len  params
+		 * example:       0x81       5-16    X    0x82 0-255    X
 		 */
-		if (skb->len < NFC_MIN_AID_LENGTH + 2 ||
-		    skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
+		if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
 			return -EPROTO;
 
-		transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);
-		if (!transaction)
-			return -ENOMEM;
-
-		transaction->aid_len = skb->data[1];
+		aid_len = skb->data[1];
 
-		/* Checking if the length of the AID is valid */
-		if (transaction->aid_len > sizeof(transaction->aid)) {
-			devm_kfree(dev, transaction);
-			return -EINVAL;
-		}
+		if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid))
+			return -EPROTO;
 
-		memcpy(transaction->aid, &skb->data[2],
-		       transaction->aid_len);
+		params_len = skb->data[aid_len + 3];
 
-		/* Check next byte is PARAMETERS tag (82) */
-		if (skb->data[transaction->aid_len + 2] !=
-		    NFC_EVT_TRANSACTION_PARAMS_TAG) {
-			devm_kfree(dev, transaction);
+		/* Verify PARAMETERS tag is (82), and final check that there is enough
+		 * space in the packet to read everything.
+		 */
+		if ((skb->data[aid_len + 2] != NFC_EVT_TRANSACTION_PARAMS_TAG) ||
+		    (skb->len < aid_len + 4 + params_len))
 			return -EPROTO;
-		}
 
-		transaction->params_len = skb->data[transaction->aid_len + 3];
+		transaction = devm_kzalloc(dev, sizeof(*transaction) + params_len, GFP_KERNEL);
+		if (!transaction)
+			return -ENOMEM;
 
-		/* Total size is allocated (skb->len - 2) minus fixed array members */
-		if (transaction->params_len > ((skb->len - 2) -
-		    sizeof(struct nfc_evt_transaction))) {
-			devm_kfree(dev, transaction);
-			return -EINVAL;
-		}
+		transaction->aid_len = aid_len;
+		transaction->params_len = params_len;
 
-		memcpy(transaction->params, skb->data +
-		       transaction->aid_len + 4, transaction->params_len);
+		memcpy(transaction->aid, &skb->data[2], aid_len);
+		memcpy(transaction->params, &skb->data[aid_len + 4], params_len);
 
 		r = nfc_se_transaction(hdev->ndev, host, transaction);
 	break;
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH net v3 3/3] nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

[Guenter Roeck]

On Mon, Jun 6, 2022 at 7:57 PM Martin Faltesek <mfaltesek@google.com> wrote:
>
> The transaction buffer is allocated by using the size of the packet buf,
> and subtracting two which seem intended to remove the two tags which are
> not present in the target structure. This calculation leads to under
> counting memory because of differences between the packet contents and the
> target structure. The aid_len field is a u8 in the packet, but a u32 in
> the structure, resulting in at least 3 bytes always being under counted.
> Further, the aid data is a variable length field in the packet, but fixed
> in the structure, so if this field is less than the max, the difference is
> added to the under counting.
>
> The last validation check for transaction->params_len is also incorrect
> since it employs the same accounting error.
>
> To fix, perform validation checks progressively to safely reach the
> next field, to determine the size of both buffers and verify both tags.
> Once all validation checks pass, allocate the buffer and copy the data.
> This eliminates freeing memory on the error path, as those checks are
> moved ahead of memory allocation.
>
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>  drivers/nfc/st21nfca/se.c | 60 +++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index 8e1113ce139b..df8d27cf2956 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -300,6 +300,8 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
>         int r = 0;
>         struct device *dev = &hdev->ndev->dev;
>         struct nfc_evt_transaction *transaction;
> +       u32 aid_len;
> +       u8 params_len;
>
>         pr_debug("connectivity gate event: %x\n", event);
>
> @@ -308,50 +310,48 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
>                 r = nfc_se_connectivity(hdev->ndev, host);
>         break;
>         case ST21NFCA_EVT_TRANSACTION:
> -               /*
> -                * According to specification etsi 102 622
> +               /* According to specification etsi 102 622
>                  * 11.2.2.4 EVT_TRANSACTION Table 52
>                  * Description  Tag     Length
>                  * AID          81      5 to 16
>                  * PARAMETERS   82      0 to 255
> +                *
> +                * The key differences are aid storage length is variably sized
> +                * in the packet, but fixed in nfc_evt_transaction, and that the aid_len
> +                * is u8 in the packet, but u32 in the structure, and the tags in
> +                * the packet are not included in nfc_evt_transaction.
> +                *
> +                * size in bytes: 1          1       5-16 1             1           0-255
> +                * offset:        0          1       2    aid_len + 2   aid_len + 3 aid_len + 4
> +                * member name:   aid_tag(M) aid_len aid  params_tag(M) params_len  params
> +                * example:       0x81       5-16    X    0x82 0-255    X
>                  */
> -               if (skb->len < NFC_MIN_AID_LENGTH + 2 ||
> -                   skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
> +               if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
>                         return -EPROTO;
>
> -               transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);
> -               if (!transaction)
> -                       return -ENOMEM;
> -
> -               transaction->aid_len = skb->data[1];
> +               aid_len = skb->data[1];
>
> -               /* Checking if the length of the AID is valid */
> -               if (transaction->aid_len > sizeof(transaction->aid)) {
> -                       devm_kfree(dev, transaction);
> -                       return -EINVAL;
> -               }
> +               if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid))
> +                       return -EPROTO;
>
> -               memcpy(transaction->aid, &skb->data[2],
> -                      transaction->aid_len);
> +               params_len = skb->data[aid_len + 3];
>
> -               /* Check next byte is PARAMETERS tag (82) */
> -               if (skb->data[transaction->aid_len + 2] !=
> -                   NFC_EVT_TRANSACTION_PARAMS_TAG) {
> -                       devm_kfree(dev, transaction);
> +               /* Verify PARAMETERS tag is (82), and final check that there is enough
> +                * space in the packet to read everything.
> +                */
> +               if ((skb->data[aid_len + 2] != NFC_EVT_TRANSACTION_PARAMS_TAG) ||
> +                   (skb->len < aid_len + 4 + params_len))
>                         return -EPROTO;
> -               }
>
> -               transaction->params_len = skb->data[transaction->aid_len + 3];
> +               transaction = devm_kzalloc(dev, sizeof(*transaction) + params_len, GFP_KERNEL);
> +               if (!transaction)
> +                       return -ENOMEM;
>
> -               /* Total size is allocated (skb->len - 2) minus fixed array members */
> -               if (transaction->params_len > ((skb->len - 2) -
> -                   sizeof(struct nfc_evt_transaction))) {
> -                       devm_kfree(dev, transaction);
> -                       return -EINVAL;
> -               }
> +               transaction->aid_len = aid_len;
> +               transaction->params_len = params_len;
>
> -               memcpy(transaction->params, skb->data +
> -                      transaction->aid_len + 4, transaction->params_len);
> +               memcpy(transaction->aid, &skb->data[2], aid_len);
> +               memcpy(transaction->params, &skb->data[aid_len + 4], params_len);
>
>                 r = nfc_se_transaction(hdev->ndev, host, transaction);
>         break;
> --
> 2.36.1.255.ge46751e96f-goog
>

Re: [PATCH net v3 3/3] nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION

[Krzysztof Kozlowski]

On 07/06/2022 04:57, Martin Faltesek wrote:
> The transaction buffer is allocated by using the size of the packet buf,
> and subtracting two which seem intended to remove the two tags which are
> not present in the target structure. This calculation leads to under
> counting memory because of differences between the packet contents and the
> target structure. The aid_len field is a u8 in the packet, but a u32 in
> the structure, resulting in at least 3 bytes always being under counted.
> Further, the aid data is a variable length field in the packet, but fixed
> in the structure, so if this field is less than the max, the difference is
> added to the under counting.
> 
> The last validation check for transaction->params_len is also incorrect
> since it employs the same accounting error.
> 
> To fix, perform validation checks progressively to safely reach the
> next field, to determine the size of both buffers and verify both tags.
> Once all validation checks pass, allocate the buffer and copy the data.
> This eliminates freeing memory on the error path, as those checks are
> moved ahead of memory allocation.
> 
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH net v3 0/3] Split "nfc: st21nfca: Refactor EVT_TRANSACTION" into 3

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  6 Jun 2022 21:57:26 -0500 you wrote:
> Change log:
> 
> v2 -> v3:
> 
> 1. v2 review comment: modified sender email to match SoB line.
> 
> 2. v2 review comment: threading emails by using git send-email as
>    recommended.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/3] nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION
    https://git.kernel.org/netdev/net/c/77e5fe8f176a
  - [net,v3,2/3] nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling
    https://git.kernel.org/netdev/net/c/996419e0594a
  - [net,v3,3/3] nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION
    https://git.kernel.org/netdev/net/c/f2e19b36593c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v3 0/3] Split "nfc: st21nfca: Refactor EVT_TRANSACTION" into 3

[Denis Efremov]

Hi,

On 6/7/22 06:57, Martin Faltesek wrote:
> 
> Martin Faltesek (3):
>   nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION
>   nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling
>   nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION
> 
>  drivers/nfc/st21nfca/se.c | 53 ++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 23 deletions(-)


It looks like driver st-nci contains the same problems and all 3 fixes are
also applicable to st_nci_hci_connectivity_event_received() function.
At least I can see the memory leak
https://elixir.bootlin.com/linux/v5.19-rc7/source/drivers/nfc/st-nci/se.c#L343

Can you please double check the st-nci driver and send the same fixes to it?
Reported-by: Denis Efremov <denis.e.efremov@oracle.com>

Thanks,
Denis

Re: [PATCH net v3 0/3] Split "nfc: st21nfca: Refactor EVT_TRANSACTION" into 3

[Martin Faltesek]

On Wed, Jul 20, 2022 at 1:25 AM Denis Efremov
<denis.e.efremov@oracle.com> wrote:
>
> Hi,
>
> On 6/7/22 06:57, Martin Faltesek wrote:
> >
> > Martin Faltesek (3):
> >   nfc: st21nfca: fix incorrect validating logic in EVT_TRANSACTION
> >   nfc: st21nfca: fix memory leaks in EVT_TRANSACTION handling
> >   nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION
> >
> >  drivers/nfc/st21nfca/se.c | 53 ++++++++++++++++++++++-----------------
> >  1 file changed, 30 insertions(+), 23 deletions(-)
>
>
> It looks like driver st-nci contains the same problems and all 3 fixes are
> also applicable to st_nci_hci_connectivity_event_received() function.
> At least I can see the memory leak
> https://elixir.bootlin.com/linux/v5.19-rc7/source/drivers/nfc/st-nci/se.c#L343
>
> Can you please double check the st-nci driver and send the same fixes to it?
> Reported-by: Denis Efremov <denis.e.efremov@oracle.com>

Will do.