* re: NFC: st21nfca: Add HCI transaction event support
@ 2015-02-10 9:00 Dan Carpenter
2015-06-05 10:59 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2015-02-10 9:00 UTC (permalink / raw)
To: christophe.ricard; +Cc: linux-wireless
Hello Christophe Ricard,
The patch 26fc6c7f02cb: "NFC: st21nfca: Add HCI transaction event
support" from Feb 1, 2015, leads to the following static checker
warning:
drivers/nfc/st21nfca/st21nfca_se.c:321 st21nfca_connectivity_event_received()
error: 'skb->data[1]' from user is not capped properly
drivers/nfc/st21nfca/st21nfca_se.c
300 int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
301 u8 event, struct sk_buff *skb)
302 {
303 int r = 0;
304 struct device *dev = &hdev->ndev->dev;
305 struct nfc_evt_transaction *transaction;
306
307 pr_debug("connectivity gate event: %x\n", event);
308
309 switch (event) {
310 case ST21NFCA_EVT_CONNECTIVITY:
311 break;
312 case ST21NFCA_EVT_TRANSACTION:
313 if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
314 skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
315 return -EPROTO;
Here we don't trust skb->data[0].
316
317 transaction = (struct nfc_evt_transaction *)devm_kzalloc(dev,
318 skb->len - 2, GFP_KERNEL);
319
320 transaction->aid_len = skb->data[1];
321 memcpy(transaction->aid, &skb->data[2], skb->data[1]);
^^^^^^^^^^^^
But here we trust skb->data[1].
NFC code is hard to analyze because sometimes skb->data[] comes from the
firmware and holds trusted values. But sometimes it comes from the
network and can overflow. Smatch marks it all as untrusted so it causes
a lot of false postives.
Some of them have comments like:
net/nfc/hci/core.c:218 nfc_hci_cmd_received()
error: buffer overflow 'hdev->pipes' 127 <= 255
But this one doesn't have a comment so it's hard for me as an outsider
to say if this is a bug or not.
322
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: NFC: st21nfca: Add HCI transaction event support
2015-02-10 9:00 NFC: st21nfca: Add HCI transaction event support Dan Carpenter
@ 2015-06-05 10:59 ` Dan Carpenter
2015-06-05 11:05 ` christophe.ricard
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2015-06-05 10:59 UTC (permalink / raw)
To: christophe.ricard; +Cc: linux-wireless
I never got a response on this. Is this remote exploitable or from the
firmware?
regards,
dan carpenter
On Tue, Feb 10, 2015 at 12:00:51PM +0300, Dan Carpenter wrote:
> Hello Christophe Ricard,
>
> The patch 26fc6c7f02cb: "NFC: st21nfca: Add HCI transaction event
> support" from Feb 1, 2015, leads to the following static checker
> warning:
>
> drivers/nfc/st21nfca/st21nfca_se.c:321 st21nfca_connectivity_event_received()
> error: 'skb->data[1]' from user is not capped properly
>
> drivers/nfc/st21nfca/st21nfca_se.c
> 300 int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
> 301 u8 event, struct sk_buff *skb)
> 302 {
> 303 int r = 0;
> 304 struct device *dev = &hdev->ndev->dev;
> 305 struct nfc_evt_transaction *transaction;
> 306
> 307 pr_debug("connectivity gate event: %x\n", event);
> 308
> 309 switch (event) {
> 310 case ST21NFCA_EVT_CONNECTIVITY:
> 311 break;
> 312 case ST21NFCA_EVT_TRANSACTION:
> 313 if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
> 314 skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
> 315 return -EPROTO;
>
> Here we don't trust skb->data[0].
>
> 316
> 317 transaction = (struct nfc_evt_transaction *)devm_kzalloc(dev,
> 318 skb->len - 2, GFP_KERNEL);
> 319
> 320 transaction->aid_len = skb->data[1];
> 321 memcpy(transaction->aid, &skb->data[2], skb->data[1]);
> ^^^^^^^^^^^^
> But here we trust skb->data[1].
>
> NFC code is hard to analyze because sometimes skb->data[] comes from the
> firmware and holds trusted values. But sometimes it comes from the
> network and can overflow. Smatch marks it all as untrusted so it causes
> a lot of false postives.
>
> Some of them have comments like:
>
> net/nfc/hci/core.c:218 nfc_hci_cmd_received()
> error: buffer overflow 'hdev->pipes' 127 <= 255
>
> But this one doesn't have a comment so it's hard for me as an outsider
> to say if this is a bug or not.
>
> 322
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: NFC: st21nfca: Add HCI transaction event support
2015-06-05 10:59 ` Dan Carpenter
@ 2015-06-05 11:05 ` christophe.ricard
0 siblings, 0 replies; 3+ messages in thread
From: christophe.ricard @ 2015-06-05 11:05 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
Hi Dan,
Actually, i am sorry, i forgot to reply back on your email but i have
tried to add comments in a follow up patch for st21nfca and st21nfcb:
st21nfca: https://lists.01.org/pipermail/linux-nfc/2015-March/003463.html
st21nfcb: https://lists.01.org/pipermail/linux-nfc/2015-March/003462.html
The actual answer from an nfc evt_transaction is a known structure with
secure element application identifier (aid) + some data.
Please let me know if it is still not clear enough.
Best Regards
On 05/06/2015 12:59, Dan Carpenter wrote:
> I never got a response on this. Is this remote exploitable or from the
> firmware?
>
> regards,
> dan carpenter
>
> On Tue, Feb 10, 2015 at 12:00:51PM +0300, Dan Carpenter wrote:
>> Hello Christophe Ricard,
>>
>> The patch 26fc6c7f02cb: "NFC: st21nfca: Add HCI transaction event
>> support" from Feb 1, 2015, leads to the following static checker
>> warning:
>>
>> drivers/nfc/st21nfca/st21nfca_se.c:321 st21nfca_connectivity_event_received()
>> error: 'skb->data[1]' from user is not capped properly
>>
>> drivers/nfc/st21nfca/st21nfca_se.c
>> 300 int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host,
>> 301 u8 event, struct sk_buff *skb)
>> 302 {
>> 303 int r = 0;
>> 304 struct device *dev = &hdev->ndev->dev;
>> 305 struct nfc_evt_transaction *transaction;
>> 306
>> 307 pr_debug("connectivity gate event: %x\n", event);
>> 308
>> 309 switch (event) {
>> 310 case ST21NFCA_EVT_CONNECTIVITY:
>> 311 break;
>> 312 case ST21NFCA_EVT_TRANSACTION:
>> 313 if (skb->len < NFC_MIN_AID_LENGTH + 2 &&
>> 314 skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG)
>> 315 return -EPROTO;
>>
>> Here we don't trust skb->data[0].
>>
>> 316
>> 317 transaction = (struct nfc_evt_transaction *)devm_kzalloc(dev,
>> 318 skb->len - 2, GFP_KERNEL);
>> 319
>> 320 transaction->aid_len = skb->data[1];
>> 321 memcpy(transaction->aid, &skb->data[2], skb->data[1]);
>> ^^^^^^^^^^^^
>> But here we trust skb->data[1].
>>
>> NFC code is hard to analyze because sometimes skb->data[] comes from the
>> firmware and holds trusted values. But sometimes it comes from the
>> network and can overflow. Smatch marks it all as untrusted so it causes
>> a lot of false postives.
>>
>> Some of them have comments like:
>>
>> net/nfc/hci/core.c:218 nfc_hci_cmd_received()
>> error: buffer overflow 'hdev->pipes' 127 <= 255
>>
>> But this one doesn't have a comment so it's hard for me as an outsider
>> to say if this is a bug or not.
>>
>> 322
>>
>> regards,
>> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-05 11:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-10 9:00 NFC: st21nfca: Add HCI transaction event support Dan Carpenter
2015-06-05 10:59 ` Dan Carpenter
2015-06-05 11:05 ` christophe.ricard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).