netdev - Re: [PATCH v3] nfc: st-nci: Fix potential buffer overflows in EVT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220112093428.58981696@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Wed, 12 Jan 2022 09:34:28 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jordy Zomer <jordy@...ing.systems>
Cc:     davem@...emloft.net, krzysztof.kozlowski@...onical.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        wengjianfeng@...ong.com
Subject: Re: [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@...ing.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;