* re: NFC: Initial support for Inside Secure microread
@ 2014-01-20 14:29 Dan Carpenter
2014-01-20 18:03 ` Marcel Holtmann
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2014-01-20 14:29 UTC (permalink / raw)
To: eric.lapuyade; +Cc: linux-wireless, linux-nfc, Johannes Berg
Hello Eric Lapuyade,
The patch cfad1ba87150: "NFC: Initial support for Inside Secure
microread" from Dec 18, 2012, leads to the following
Sparse warning:
drivers/nfc/microread/microread.c:534:25:
warning: cast to restricted __le16"
drivers/nfc/microread/microread.c
512 case MICROREAD_GATE_ID_MREAD_ISO_A_3:
513 targets->supported_protocols =
514 nfc_hci_sak_to_protocol(skb->data[MICROREAD_EMCF_A3_SAK]);
515 targets->sens_res =
516 be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
^^^^^^^^^^^
sens_res is network (big) endian here. We could silence the sparse
warning by doing:
argets->sens_res =
ntohs(*(__be16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
517 targets->sel_res = skb->data[MICROREAD_EMCF_A3_SAK];
518 targets->nfcid1_len = skb->data[MICROREAD_EMCF_A3_LEN];
519 if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
520 r = -EINVAL;
521 goto exit_free;
522 }
523 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
524 targets->nfcid1_len);
525 break;
526 case MICROREAD_GATE_ID_MREAD_ISO_B:
527 targets->supported_protocols = NFC_PROTO_ISO14443_B_MASK;
528 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_B_UID], 4);
529 targets->nfcid1_len = 4;
530 break;
531 case MICROREAD_GATE_ID_MREAD_NFC_T1:
532 targets->supported_protocols = NFC_PROTO_JEWEL_MASK;
533 targets->sens_res =
534 le16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_T1_ATQA]);
^^^^^^^^^^^
But here it is little endian. This seems like either a horrible
protocol or a bug in the code.
535 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_T1_UID], 4);
536 targets->nfcid1_len = 4;
537 break;
Btw, we also don't seem to check if we are reading beyond the end of
skb->len. This would only be a problem if we got unlucky and skb->data
were allocated at the end of page. The read beyond the end of the page
would cause an oops.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: NFC: Initial support for Inside Secure microread
2014-01-20 14:29 NFC: Initial support for Inside Secure microread Dan Carpenter
@ 2014-01-20 18:03 ` Marcel Holtmann
0 siblings, 0 replies; 2+ messages in thread
From: Marcel Holtmann @ 2014-01-20 18:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: eric.lapuyade, linux-wireless@vger.kernel.org Wireless,
linux-nfc@ml01.01.org NFC, Johannes Berg
Hi Dan,
> The patch cfad1ba87150: "NFC: Initial support for Inside Secure
> microread" from Dec 18, 2012, leads to the following
> Sparse warning:
>
> drivers/nfc/microread/microread.c:534:25:
> warning: cast to restricted __le16"
>
> drivers/nfc/microread/microread.c
> 512 case MICROREAD_GATE_ID_MREAD_ISO_A_3:
> 513 targets->supported_protocols =
> 514 nfc_hci_sak_to_protocol(skb->data[MICROREAD_EMCF_A3_SAK]);
> 515 targets->sens_res =
> 516 be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
> ^^^^^^^^^^^
> sens_res is network (big) endian here. We could silence the sparse
> warning by doing:
>
> argets->sens_res =
> ntohs(*(__be16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
wouldn't it be easier to just use get_unaligned_be16. I am not even sure that we actually have a proper alignment on all architecture here. So this might be safer anyway. Eric?
>
> 517 targets->sel_res = skb->data[MICROREAD_EMCF_A3_SAK];
> 518 targets->nfcid1_len = skb->data[MICROREAD_EMCF_A3_LEN];
> 519 if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
> 520 r = -EINVAL;
> 521 goto exit_free;
> 522 }
> 523 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
> 524 targets->nfcid1_len);
> 525 break;
> 526 case MICROREAD_GATE_ID_MREAD_ISO_B:
> 527 targets->supported_protocols = NFC_PROTO_ISO14443_B_MASK;
> 528 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_B_UID], 4);
> 529 targets->nfcid1_len = 4;
> 530 break;
> 531 case MICROREAD_GATE_ID_MREAD_NFC_T1:
> 532 targets->supported_protocols = NFC_PROTO_JEWEL_MASK;
> 533 targets->sens_res =
> 534 le16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_T1_ATQA]);
> ^^^^^^^^^^^
> But here it is little endian. This seems like either a horrible
> protocol or a bug in the code.
>
> 535 memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_T1_UID], 4);
> 536 targets->nfcid1_len = 4;
> 537 break;
>
> Btw, we also don't seem to check if we are reading beyond the end of
> skb->len. This would only be a problem if we got unlucky and skb->data
> were allocated at the end of page. The read beyond the end of the page
> would cause an oops.
Regards
Marcel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-01-20 18:03 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-20 14:29 NFC: Initial support for Inside Secure microread Dan Carpenter
2014-01-20 18:03 ` Marcel Holtmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox