Re: [PATCH v2] nfc: st-nci: Fix potential buffer overflows in EVT_TRANSACTION

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 18/11/2021 08:02, 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.
> 
> It would be nice if someone can review and test this patch because
> I don't own the hardware :)

Thanks for your patch.
You mentioned that there are buffer overflows but you do not own the
hardware. How do you know these overflow exist? How did you detect them?

> 
> EDIT: Changed comment style and double newlines



Please add changelog after --- separators so it does not clutter the
commit log with unrelated "EDIT".

> 
> Signed-off-by: Jordy Zomer <jordy@pwning.systems>
> ---
>  drivers/nfc/st-nci/se.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index 7764b1a4c3cf..8e2ac8a3d199 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -335,6 +335,11 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>  			return -ENOMEM;
>  
>  		transaction->aid_len = skb->data[1];
> +
> +		/* Checking if the length of the AID is valid */
> +		if (transaction->aid_len > sizeof(transaction->aid))
> +			return -EINVAL;

I am thinking whether the check should be before memory allocation - to
save on useless memory allocation in case of error, but make the code
less obvious with referring to skb->data[1] twice.

> +
>  		memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
>  
>  		/* Check next byte is PARAMETERS tag (82) */
> @@ -343,6 +348,16 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
>  			return -EPROTO;
>  
>  		transaction->params_len = skb->data[transaction->aid_len + 3];
> +
> +		/*
> +		 * check if the length of the parameters is valid
> +		 * we can't use sizeof(transaction->params) because it's
> +		 * a flexible array member so we have to check if params_len
> +		 * is bigger than the space allocated for the array
> +		 */
> +		if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
> +			return -EINVAL;

The current comment is long and actually not explaining how you get "-2"
and sizeof, so how about:
"Total size is allocated (skb->len - 2) minus fixed array members)"

In general the code looks ok, although I cannot provide tests.


> +
>  		memcpy(transaction->params, skb->data +
>  		       transaction->aid_len + 4, transaction->params_len);
>  
> 


Best regards,
Krzysztof