* [patch -RESEND] NFC: potential overflows in microread_target_discovered()
@ 2014-09-01 17:27 Dan Carpenter
2014-09-02 7:02 ` Frans Klaver
2014-09-04 23:03 ` Samuel Ortiz
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2014-09-01 17:27 UTC (permalink / raw)
To: Lauro Ramos Venancio, Eric Lapuyade
Cc: Aloisio Almeida Jr, Samuel Ortiz, John W. Linville, Jeff Kirsher,
linux-wireless, linux-nfc, kernel-janitors, Kees Cook
Smatch says that skb->data is untrusted so we need to check to make sure
that the memcpy() doesn't overflow.
Fixes: cfad1ba87150 ('NFC: Initial support for Inside Secure microread')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I sent this in January but never received any response. I don't know
the subsystem very well but it looks like it could have security
implications.
Compile tested only.
diff --git a/drivers/nfc/microread/microread.c b/drivers/nfc/microread/microread.c
index f868333271aa..963a4a5dc88e 100644
--- a/drivers/nfc/microread/microread.c
+++ b/drivers/nfc/microread/microread.c
@@ -501,9 +501,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
targets->sens_res =
be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A_ATQA]);
targets->sel_res = skb->data[MICROREAD_EMCF_A_SAK];
- memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
- skb->data[MICROREAD_EMCF_A_LEN]);
targets->nfcid1_len = skb->data[MICROREAD_EMCF_A_LEN];
+ if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
+ r = -EINVAL;
+ goto exit_free;
+ }
+ memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
+ targets->nfcid1_len);
break;
case MICROREAD_GATE_ID_MREAD_ISO_A_3:
targets->supported_protocols =
@@ -511,9 +515,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
targets->sens_res =
be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
targets->sel_res = skb->data[MICROREAD_EMCF_A3_SAK];
- memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
- skb->data[MICROREAD_EMCF_A3_LEN]);
targets->nfcid1_len = skb->data[MICROREAD_EMCF_A3_LEN];
+ if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
+ r = -EINVAL;
+ goto exit_free;
+ }
+ memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
+ targets->nfcid1_len);
break;
case MICROREAD_GATE_ID_MREAD_ISO_B:
targets->supported_protocols = NFC_PROTO_ISO14443_B_MASK;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch -RESEND] NFC: potential overflows in microread_target_discovered()
2014-09-01 17:27 [patch -RESEND] NFC: potential overflows in microread_target_discovered() Dan Carpenter
@ 2014-09-02 7:02 ` Frans Klaver
2014-09-02 8:54 ` Dan Carpenter
2014-09-04 23:03 ` Samuel Ortiz
1 sibling, 1 reply; 5+ messages in thread
From: Frans Klaver @ 2014-09-02 7:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lauro Ramos Venancio, Eric Lapuyade, Aloisio Almeida Jr,
Samuel Ortiz, John W. Linville, Jeff Kirsher, linux-wireless,
linux-nfc, kernel-janitors, Kees Cook
Hi,
On Mon, Sep 1, 2014 at 7:27 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Smatch says that skb->data is untrusted so we need to check to make sure
> that the memcpy() doesn't overflow.
>
> Fixes: cfad1ba87150 ('NFC: Initial support for Inside Secure microread')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I sent this in January but never received any response. I don't know
> the subsystem very well but it looks like it could have security
> implications.
>
> Compile tested only.
>
> diff --git a/drivers/nfc/microread/microread.c b/drivers/nfc/microread/microread.c
> index f868333271aa..963a4a5dc88e 100644
> --- a/drivers/nfc/microread/microread.c
> +++ b/drivers/nfc/microread/microread.c
> @@ -501,9 +501,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
> targets->sens_res =
> be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A_ATQA]);
> targets->sel_res = skb->data[MICROREAD_EMCF_A_SAK];
> - memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
> - skb->data[MICROREAD_EMCF_A_LEN]);
> targets->nfcid1_len = skb->data[MICROREAD_EMCF_A_LEN];
> + if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
You should probably compare against sizeof(*targets->nfcid1).
> + r = -EINVAL;
> + goto exit_free;
> + }
> + memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
> + targets->nfcid1_len);
> break;
> case MICROREAD_GATE_ID_MREAD_ISO_A_3:
> targets->supported_protocols =
> @@ -511,9 +515,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
> targets->sens_res =
> be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A3_ATQA]);
> targets->sel_res = skb->data[MICROREAD_EMCF_A3_SAK];
> - memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
> - skb->data[MICROREAD_EMCF_A3_LEN]);
> targets->nfcid1_len = skb->data[MICROREAD_EMCF_A3_LEN];
> + if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
Same here.
> + r = -EINVAL;
> + goto exit_free;
> + }
> + memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A3_UID],
> + targets->nfcid1_len);
> break;
> case MICROREAD_GATE_ID_MREAD_ISO_B:
> targets->supported_protocols = NFC_PROTO_ISO14443_B_MASK;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch -RESEND] NFC: potential overflows in microread_target_discovered()
2014-09-02 7:02 ` Frans Klaver
@ 2014-09-02 8:54 ` Dan Carpenter
2014-09-02 8:57 ` Frans Klaver
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2014-09-02 8:54 UTC (permalink / raw)
To: Frans Klaver
Cc: Lauro Ramos Venancio, Eric Lapuyade, Aloisio Almeida Jr,
Samuel Ortiz, John W. Linville, Jeff Kirsher, linux-wireless,
linux-nfc, kernel-janitors, Kees Cook
On Tue, Sep 02, 2014 at 09:02:36AM +0200, Frans Klaver wrote:
> > diff --git a/drivers/nfc/microread/microread.c b/drivers/nfc/microread/microread.c
> > index f868333271aa..963a4a5dc88e 100644
> > --- a/drivers/nfc/microread/microread.c
> > +++ b/drivers/nfc/microread/microread.c
> > @@ -501,9 +501,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
> > targets->sens_res =
> > be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A_ATQA]);
> > targets->sel_res = skb->data[MICROREAD_EMCF_A_SAK];
> > - memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
> > - skb->data[MICROREAD_EMCF_A_LEN]);
> > targets->nfcid1_len = skb->data[MICROREAD_EMCF_A_LEN];
> > + if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
>
> You should probably compare against sizeof(*targets->nfcid1).
>
No. It's an array not a pointer.
You should make a small test program to test your ideas.
int main(void)
{
char buf[10];
printf("%d %d\n", sizeof(buf), sizeof(*buf));
return 0;
}
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch -RESEND] NFC: potential overflows in microread_target_discovered()
2014-09-02 8:54 ` Dan Carpenter
@ 2014-09-02 8:57 ` Frans Klaver
0 siblings, 0 replies; 5+ messages in thread
From: Frans Klaver @ 2014-09-02 8:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lauro Ramos Venancio, Eric Lapuyade, Aloisio Almeida Jr,
Samuel Ortiz, John W. Linville, Jeff Kirsher, linux-wireless,
linux-nfc, kernel-janitors, Kees Cook
On Tue, Sep 2, 2014 at 10:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 02, 2014 at 09:02:36AM +0200, Frans Klaver wrote:
>> > diff --git a/drivers/nfc/microread/microread.c b/drivers/nfc/microread/microread.c
>> > index f868333271aa..963a4a5dc88e 100644
>> > --- a/drivers/nfc/microread/microread.c
>> > +++ b/drivers/nfc/microread/microread.c
>> > @@ -501,9 +501,13 @@ static void microread_target_discovered(struct nfc_hci_dev *hdev, u8 gate,
>> > targets->sens_res =
>> > be16_to_cpu(*(u16 *)&skb->data[MICROREAD_EMCF_A_ATQA]);
>> > targets->sel_res = skb->data[MICROREAD_EMCF_A_SAK];
>> > - memcpy(targets->nfcid1, &skb->data[MICROREAD_EMCF_A_UID],
>> > - skb->data[MICROREAD_EMCF_A_LEN]);
>> > targets->nfcid1_len = skb->data[MICROREAD_EMCF_A_LEN];
>> > + if (targets->nfcid1_len > sizeof(targets->nfcid1)) {
>>
>> You should probably compare against sizeof(*targets->nfcid1).
>>
>
> No. It's an array not a pointer.
Ai, I overlooked that one. My bad.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch -RESEND] NFC: potential overflows in microread_target_discovered()
2014-09-01 17:27 [patch -RESEND] NFC: potential overflows in microread_target_discovered() Dan Carpenter
2014-09-02 7:02 ` Frans Klaver
@ 2014-09-04 23:03 ` Samuel Ortiz
1 sibling, 0 replies; 5+ messages in thread
From: Samuel Ortiz @ 2014-09-04 23:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lauro Ramos Venancio, Eric Lapuyade, Aloisio Almeida Jr,
John W. Linville, Jeff Kirsher, linux-wireless, linux-nfc,
kernel-janitors, Kees Cook
Hi Dan,
On Mon, Sep 01, 2014 at 08:27:29PM +0300, Dan Carpenter wrote:
> Smatch says that skb->data is untrusted so we need to check to make sure
> that the memcpy() doesn't overflow.
Applied to nfc-fixes, thanks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-04 23:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-01 17:27 [patch -RESEND] NFC: potential overflows in microread_target_discovered() Dan Carpenter
2014-09-02 7:02 ` Frans Klaver
2014-09-02 8:54 ` Dan Carpenter
2014-09-02 8:57 ` Frans Klaver
2014-09-04 23:03 ` Samuel Ortiz
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).