diff mbox

[-RESEND] NFC: potential overflows in microread_target_discovered()

Message ID 20140901172729.GA6549@mwanda (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Dan Carpenter Sept. 1, 2014, 5:27 p.m. UTC 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Frans Klaver Sept. 2, 2014, 7:02 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Sept. 2, 2014, 8:54 a.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frans Klaver Sept. 2, 2014, 8:57 a.m. UTC | #3 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Samuel Ortiz Sept. 4, 2014, 11:03 p.m. UTC | #4 
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.
diff mbox

Patch

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;