Re: [PATCH v2] nfc: st-nci: Fix potential buffer overflows in EVT_TRANSACTION
From: Krzysztof Kozlowski <hidden>
Date: 2021-11-18 07:36:34
Also in:
lkml
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".
quoted hunk ↗ jump to hunk
Signed-off-by: Jordy Zomer <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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