[PATCH net 0/3] nfc: st-nci: Restructure validating logic in EVT_TRANSACTION

[PATCH net 0/3] nfc: st-nci: Restructure validating logic in EVT_TRANSACTION

[Martin Faltesek]

These are the same 3 patches that were applied in st21nfca here:
https://lore.kernel.org/netdev/20220607025729.1673212-1-mfaltesek@google.com
with a couple minor differences.

st-nci has nearly identical code to that of st21nfca for EVT_TRANSACTION, except that there
are two extra validation checks that are not present in the st-nci code. The 3/3 patch as coded
for st21nfca pulls those checks in, bringing both drivers into parity.

Martin Faltesek (3):
  nfc: st-nci: fix incorrect validating logic in EVT_TRANSACTION
  nfc: st-nci: fix memory leaks in EVT_TRANSACTION
  nfc: st-nci: fix incorrect sizing calculations in EVT_TRANSACTION

 drivers/nfc/st-nci/se.c | 45 +++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 13 deletions(-)


base-commit: 2360f9b8c4e81d242d4cbf99d630a2fffa681fab
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH net 1/3] nfc: st-nci: 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 ||.

Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st-nci/se.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 7764b1a4c3cf..589e1dec78e7 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -326,7 +326,7 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 		 * 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.38.1.584.g0f3c55d4c2-goog

[PATCH net 2/3] nfc: st-nci: fix memory leaks in EVT_TRANSACTION

[Martin Faltesek]

Error path does not free previously allocated memory. Add devm_kfree() to
the failure path.

Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st-nci/se.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index 589e1dec78e7..fc59916ae5ae 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -339,8 +339,10 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 
 		/* 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];
 		memcpy(transaction->params, skb->data +
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH net 3/3] nfc: st-nci: 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 seems 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.

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 validation checks are
moved ahead of memory allocation.

Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
 drivers/nfc/st-nci/se.c | 47 ++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index fc59916ae5ae..0c24f4a5c92e 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -312,6 +312,8 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 	int r = 0;
 	struct device *dev = &ndev->nfc_dev->dev;
 	struct nfc_evt_transaction *transaction;
+	u32 aid_len;
+	u8 params_len;
 
 	pr_debug("connectivity gate event: %x\n", event);
 
@@ -325,28 +327,43 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
 		 * 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;
+		aid_len = skb->data[1];
 
-		transaction->aid_len = skb->data[1];
-		memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
+		if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid))
+			return -EPROTO;
 
-		/* Check next byte is PARAMETERS tag (82) */
-		if (skb->data[transaction->aid_len + 2] !=
-		    NFC_EVT_TRANSACTION_PARAMS_TAG) {
-			devm_kfree(dev, transaction);
+		params_len = skb->data[aid_len + 3];
+
+		/* 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];
-		memcpy(transaction->params, skb->data +
-		       transaction->aid_len + 4, transaction->params_len);
+		transaction = devm_kzalloc(dev, sizeof(*transaction) + params_len, GFP_KERNEL);
+		if (!transaction)
+			return -ENOMEM;
+
+		transaction->aid_len = aid_len;
+		transaction->params_len = params_len;
+
+		memcpy(transaction->aid, &skb->data[2], aid_len);
+		memcpy(transaction->params, &skb->data[aid_len + 4], params_len);
 
 		r = nfc_se_transaction(ndev->nfc_dev, host, transaction);
 		break;
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH net 1/3] nfc: st-nci: fix incorrect validating logic in EVT_TRANSACTION

[Guenter Roeck]

On Fri, Nov 18, 2022 at 2:04 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 ||.
>
> Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
> Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Reviewed-by: Guenter Roeck <groeck@google.com>

> ---
>  drivers/nfc/st-nci/se.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index 7764b1a4c3cf..589e1dec78e7 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -326,7 +326,7 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>                  * 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.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH net 2/3] nfc: st-nci: fix memory leaks in EVT_TRANSACTION

[Guenter Roeck]

On Fri, Nov 18, 2022 at 2:04 PM Martin Faltesek <mfaltesek@google.com> wrote:
>
> Error path does not free previously allocated memory. Add devm_kfree() to
> the failure path.
>
> Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
> Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Reviewed-by: Guenter Roeck <groeck@google.com>

> ---
>  drivers/nfc/st-nci/se.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index 589e1dec78e7..fc59916ae5ae 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -339,8 +339,10 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>
>                 /* 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];
>                 memcpy(transaction->params, skb->data +
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH net 3/3] nfc: st-nci: fix incorrect sizing calculations in EVT_TRANSACTION

[Guenter Roeck]

On Fri, Nov 18, 2022 at 2:04 PM Martin Faltesek <mfaltesek@google.com> wrote:
>
> The transaction buffer is allocated by using the size of the packet buf,
> and subtracting two which seems 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.
>
> 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 validation checks are
> moved ahead of memory allocation.
>
> Reported-by: Denis Efremov <denis.e.efremov@oracle.com>
> Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>

Nit below, otherwise

Reviewed-by: Guenter Roeck <groeck@google.com>

> ---
>  drivers/nfc/st-nci/se.c | 47 ++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index fc59916ae5ae..0c24f4a5c92e 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -312,6 +312,8 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>         int r = 0;
>         struct device *dev = &ndev->nfc_dev->dev;
>         struct nfc_evt_transaction *transaction;
> +       u32 aid_len;
> +       u8 params_len;
>
>         pr_debug("connectivity gate event: %x\n", event);
>
> @@ -325,28 +327,43 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>                  * 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;
> +               aid_len = skb->data[1];
>
> -               transaction->aid_len = skb->data[1];
> -               memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
> +               if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid))
> +                       return -EPROTO;
>
> -               /* Check next byte is PARAMETERS tag (82) */
> -               if (skb->data[transaction->aid_len + 2] !=
> -                   NFC_EVT_TRANSACTION_PARAMS_TAG) {
> -                       devm_kfree(dev, transaction);
> +               params_len = skb->data[aid_len + 3];
> +
> +               /* 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))

Unnecessary () after ||

>                         return -EPROTO;
> -               }
>
> -               transaction->params_len = skb->data[transaction->aid_len + 3];
> -               memcpy(transaction->params, skb->data +
> -                      transaction->aid_len + 4, transaction->params_len);
> +               transaction = devm_kzalloc(dev, sizeof(*transaction) + params_len, GFP_KERNEL);
> +               if (!transaction)
> +                       return -ENOMEM;
> +
> +               transaction->aid_len = aid_len;
> +               transaction->params_len = params_len;
> +
> +               memcpy(transaction->aid, &skb->data[2], aid_len);
> +               memcpy(transaction->params, &skb->data[aid_len + 4], params_len);
>
>                 r = nfc_se_transaction(ndev->nfc_dev, host, transaction);
>                 break;
> --
> 2.38.1.584.g0f3c55d4c2-goog
>