| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Nov 2022 14:18:16 -0800
From: Guenter Roeck <groeck@...gle.com>
To: Martin Faltesek <mfaltesek@...gle.com>
Cc: kuba@...nel.org, netdev@...r.kernel.org, linux-nfc@...ts.01.org,
krzysztof.kozlowski@...aro.org, davem@...emloft.net,
martin.faltesek@...il.com, christophe.ricard@...il.com,
jordy@...ing.systems, krzk@...nel.org, sameo@...ux.intel.com,
theflamefire89@...il.com, duoming@....edu.cn,
Denis Efremov <denis.e.efremov@...cle.com>
Subject: Re: [PATCH net 3/3] nfc: st-nci: fix incorrect sizing calculations in EVT_TRANSACTION
On Fri, Nov 18, 2022 at 2:04 PM Martin Faltesek <mfaltesek@...gle.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@...cle.com>
> Fixes: 5d1ceb7f5e56 ("NFC: st21nfcb: Add HCI transaction event support")
> Signed-off-by: Martin Faltesek <mfaltesek@...gle.com>
Nit below, otherwise
Reviewed-by: Guenter Roeck <groeck@...gle.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
>
Powered by blists - more mailing lists