lkml.org 
[lkml]   [2022]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] nfc: st-nci: Fix potential buffer overflows in EVT_TRANSACTION
On Tue, 11 Jan 2022 17:45:43 +0100 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>

This patch with more context:

> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index 7764b1a4c3cf..cdb59ddff4e8 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -333,18 +333,28 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
> transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL);

What checks skb->len > 2 ?

> if (!transaction)
> return -ENOMEM;

Leaks skb ?

> 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);

What checks skb->len > 2 + transaction->aid_len ?

> /* Check next byte is PARAMETERS tag (82) */
> if (skb->data[transaction->aid_len + 2] !=

.. make that skb->len > 2 + transaction->aid_len + 1

> NFC_EVT_TRANSACTION_PARAMS_TAG)
> return -EPROTO;

Leaks skb ? (btw devm_kmalloc() in message processing could probably as well be counted
as leak unless something guarantees attacker can't generate infinite messages of this type)

> transaction->params_len = skb->data[transaction->aid_len + 3];

.. skb->len > 2 + transaction->aid_len + 1 + 1

> + /* Total size is allocated (skb->len - 2) minus fixed array members */
> + if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))

So this check makes sure we don't overflow transaction->params, right?
Again, does skb->len not have to be validated as well?

> + return -EINVAL;
> +
> memcpy(transaction->params, skb->data +
> transaction->aid_len + 4, transaction->params_len);
>
> r = nfc_se_transaction(ndev->nfc_dev, host, transaction);
> break;

\
 
 \ /
  Last update: 2022-01-12 18:35    [W:0.083 / U:0.884 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site