* [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION
@ 2022-03-29 17:54 Martin Faltesek
2022-03-29 18:04 ` Guenter Roeck
2022-03-30 1:25 ` Jakub Kicinski
0 siblings, 2 replies; 3+ messages in thread
From: Martin Faltesek @ 2022-03-29 17:54 UTC (permalink / raw)
To: netdev, krzk, christophe.ricard, jordy
Cc: sameo, wklin, groeck, surenb, mfaltesek, gregkh
EVT_TRANSACTION has four different bugs:
1. First conditional has logical AND but should be OR. It should
always check if it isn't NFC_EVT_TRANSACTION_AID_TAG, then
bail.
2. Potential under allocating memory:devm_kzalloc (skb->len - 2)
when the aid_len specified in the packet is less than the fixed
NFC_MAX_AID_LENGTH in struct nfc_evt_transaction. In addition,
aid_len is u32 in the data structure, and u8 in the packet,
under counting 3 more bytes.
3. Memory leaks after kzalloc when returning error.
4. The final conditional check is also incorrect, for the same reasons
explained in #2.
Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
Signed-off-by: Martin Faltesek <mfaltesek@google.com>
---
drivers/nfc/st21nfca/se.c | 65 ++++++++++++++++++++++++++-------------
1 file changed, 43 insertions(+), 22 deletions(-)
diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index c922f10d0d7b..acc8d831246a 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -292,6 +292,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);
@@ -306,37 +308,56 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
* 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 part of nfc_evt_transaction.
+ *
+ * size in bytes: 1 1 5-16 1 1 0-255
+ * offset: 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)
- return -EPROTO;
-
- transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);
- if (!transaction)
- return -ENOMEM;
- transaction->aid_len = skb->data[1];
+ /*
+ * Validate the packet is large enough to read the first two bytes
+ * containing the aid_tag and aid_len, and then read both. Capacity
+ * checks are expanded incrementally after this, for clarity.
+ */
+ if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
+ return -EPROTO;
- /* Checking if the length of the AID is valid */
- if (transaction->aid_len > sizeof(transaction->aid))
- return -EINVAL;
+ aid_len = skb->data[1];
+ /*
+ * With the actual aid_len, verify there is enough space in
+ * the packet to read params_tag and params_len, and that
+ * aid_len does not exceed destination capacity. Reference
+ * offset comment above for +4 +3 +2 offsets used.
+ */
+ 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)
+ /*
+ * Verify PARAMETERS tag is (82), and final validation that enough
+ * space in 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(struct nfc_evt_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)))
- 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.35.1.1021.g381101b075-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION
2022-03-29 17:54 [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION Martin Faltesek
@ 2022-03-29 18:04 ` Guenter Roeck
2022-03-30 1:25 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2022-03-29 18:04 UTC (permalink / raw)
To: Martin Faltesek
Cc: netdev, Krzysztof Kozlowski, christophe.ricard, jordy, sameo,
William K Lin, Suren Baghdasaryan, Martin Faltesek,
Greg Kroah-Hartman
On Tue, Mar 29, 2022 at 10:54 AM Martin Faltesek <mfaltesek@chromium.org> wrote:
>
> EVT_TRANSACTION has four different bugs:
>
> 1. First conditional has logical AND but should be OR. It should
> always check if it isn't NFC_EVT_TRANSACTION_AID_TAG, then
> bail.
>
> 2. Potential under allocating memory:devm_kzalloc (skb->len - 2)
> when the aid_len specified in the packet is less than the fixed
> NFC_MAX_AID_LENGTH in struct nfc_evt_transaction. In addition,
> aid_len is u32 in the data structure, and u8 in the packet,
> under counting 3 more bytes.
>
> 3. Memory leaks after kzalloc when returning error.
>
> 4. The final conditional check is also incorrect, for the same reasons
> explained in #2.
>
> Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support")
> Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION")
> Signed-off-by: Martin Faltesek <mfaltesek@google.com>
You'll probably be asked to use networking-style multi-line comments.
Other than that, LGTM.
Reviewed-by: Guenter Roeck <groeck@google.com>
Guenter
> ---
> drivers/nfc/st21nfca/se.c | 65 ++++++++++++++++++++++++++-------------
> 1 file changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
> index c922f10d0d7b..acc8d831246a 100644
> --- a/drivers/nfc/st21nfca/se.c
> +++ b/drivers/nfc/st21nfca/se.c
> @@ -292,6 +292,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);
>
> @@ -306,37 +308,56 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
> * 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 part of nfc_evt_transaction.
> + *
> + * size in bytes: 1 1 5-16 1 1 0-255
> + * offset: 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)
> - return -EPROTO;
> -
> - transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);
> - if (!transaction)
> - return -ENOMEM;
>
> - transaction->aid_len = skb->data[1];
> + /*
> + * Validate the packet is large enough to read the first two bytes
> + * containing the aid_tag and aid_len, and then read both. Capacity
> + * checks are expanded incrementally after this, for clarity.
> + */
> + if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
> + return -EPROTO;
>
> - /* Checking if the length of the AID is valid */
> - if (transaction->aid_len > sizeof(transaction->aid))
> - return -EINVAL;
> + aid_len = skb->data[1];
> + /*
> + * With the actual aid_len, verify there is enough space in
> + * the packet to read params_tag and params_len, and that
> + * aid_len does not exceed destination capacity. Reference
> + * offset comment above for +4 +3 +2 offsets used.
> + */
> + 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)
> + /*
> + * Verify PARAMETERS tag is (82), and final validation that enough
> + * space in 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(struct nfc_evt_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)))
> - 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.35.1.1021.g381101b075-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION
2022-03-29 17:54 [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION Martin Faltesek
2022-03-29 18:04 ` Guenter Roeck
@ 2022-03-30 1:25 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2022-03-30 1:25 UTC (permalink / raw)
To: Martin Faltesek
Cc: netdev, krzk, christophe.ricard, jordy, sameo, wklin, groeck,
surenb, mfaltesek, gregkh
On Tue, 29 Mar 2022 12:54:31 -0500 Martin Faltesek wrote:
> EVT_TRANSACTION has four different bugs:
>
> 1. First conditional has logical AND but should be OR. It should
> always check if it isn't NFC_EVT_TRANSACTION_AID_TAG, then
> bail.
>
> 2. Potential under allocating memory:devm_kzalloc (skb->len - 2)
> when the aid_len specified in the packet is less than the fixed
> NFC_MAX_AID_LENGTH in struct nfc_evt_transaction. In addition,
> aid_len is u32 in the data structure, and u8 in the packet,
> under counting 3 more bytes.
>
> 3. Memory leaks after kzalloc when returning error.
>
> 4. The final conditional check is also incorrect, for the same reasons
> explained in #2.
Any time you're tempted to write a list in your commit message the
chances are you should split the change into multiple patches.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-30 1:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 17:54 [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION Martin Faltesek
2022-03-29 18:04 ` Guenter Roeck
2022-03-30 1:25 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.