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 :)
Signed-off-by: Jordy Zomer <[email protected]>
---
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..0d22853925d8 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -335,6 +335,12 @@ 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;
+
+
memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
/* Check next byte is PARAMETERS tag (82) */
@@ -343,6 +349,15 @@ 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;
+
+
memcpy(transaction->params, skb->data +
transaction->aid_len + 4, transaction->params_len);
--
2.27.0
On Wed, 17 Nov 2021 18:15:51 +0100 Jordy Zomer wrote:
> +
> + // Checking if the length of the AID is valid
> + if (transaction->aid_len > sizeof(transaction->aid))
> + return -EINVAL;
> +
> +
Please remove the double blank lines and use more common style of
multi-line comments /* */ like the rest of this file.
Same for the other patch. Thanks!
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 :)
EDIT: Changed comment style and double newlines
Signed-off-by: Jordy Zomer <[email protected]>
---
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;
+
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;
+
memcpy(transaction->params, skb->data +
transaction->aid_len + 4, transaction->params_len);
--
2.27.0
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
drivers/nfc/st-nci/se.c | 10 ++++++++++
1 file changed, 10 insertions(+)
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
@@ -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;
+
memcpy(transaction->aid, &skb->data[2], transaction->aid_len);
/* Check next byte is PARAMETERS tag (82) */
@@ -343,6 +348,11 @@ static int st_nci_hci_connectivity_event_received(struct nci_dev *ndev,
return -EPROTO;
transaction->params_len = skb->data[transaction->aid_len + 3];
+
+ /* Total size is allocated (skb->len - 2) minus fixed array members */
+ if (transaction->params_len > ((skb->len - 2) - sizeof(struct nfc_evt_transaction)))
+ return -EINVAL;
+
memcpy(transaction->params, skb->data +
transaction->aid_len + 4, transaction->params_len);
--
2.27.0
On 11/01/2022 17:45, 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 <[email protected]>
> ---
> drivers/nfc/st-nci/se.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
Looks ok.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
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;