1027479 - [NFC] Emulator support for reading NDEF data from type 4 tag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi][:dlee]]

Emulator should support processing type 4 tag
- reading function should be implemented in this bug

Comment 1

[Dimi Lee [:dimi][:dlee]]

Attachment: Part1. Add type4 tag as a remote endpoint v1

Comment 2

[Dimi Lee [:dimi][:dlee]]

Attachment: Part2. Process type 4 tag read ndef command v1

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8442666 [details] [diff] [review]
Part1. Add type4 tag as a remote endpoint v1

Review of attachment 8442666 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of the potential buffer overflow in |set_t4_data|.

::: hw/nfc-tag.c
@@ +136,5 @@
> +set_t4t_data(struct nfc_tag* tag, const uint8_t* ndef_msg, ssize_t len)
> +{
> +    assert(tag);
> +    assert(ndef_msg);
> +    assert(len < sizeof(tag->t.t4.format.data));

According to [T4TOP] Table 4., the length in data includes the length field itself. This test and code below doesn't seem to handle that correctly.

@@ +141,5 @@
> +
> +    tag->t.t4.format.data[0] = (len >> 8) & 0xff;
> +    tag->t.t4.format.data[1] = len & 0xff;
> +
> +    memcpy(tag->t.t4.format.data + 2 , ndef_msg, len);

I'm curious. Won't this memcpy override most of the defined fields in data with plain NDEF data? What is contained in |ndef_msg|?

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8442667 [details] [diff] [review]
Part2. Process type 4 tag read ndef command v1

Review of attachment 8442667 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I'm curious if the parser code is generic enough for general use.

::: hw/nfc-tag.c
@@ +61,5 @@
> +/* [T4TOP] Table 16 */
> +static const uint8_t t4t_rb_apdu[2] = { 0x00, 0xb0 };
> +
> +/* [T4TOP] Table 19 */
> +static const uint8_t t4t_ndef_apdu[5] = { 0x00, 0xa4, 0x00, 0x0c, 0x02 };

According to [T4TOP] Table 19 and 20, the final field is the length of the following data. That means you  cannot hard code it here and use it for the memcmp in |process_t4t|.

@@ +422,5 @@
> +    offset = (cmd->p1 & 0xff) << 8 | (cmd->p2 & 0xff);
> +
> +    switch (t4t_file_sel) {
> +        case CC_SELECT:
> +            memcpy(rsp->data, mem->cc + offset, cmd->le);

Are these fields large enough for any value of |cmd->le|?

@@ +484,5 @@
> +        len = process_t4t_ndef_select(&cmd->ndef_sel_cmd, consumed,
> +                                      &rsp->ndef_sel_rsp);
> +    } else {
> +        assert(0);
> +    }

Please see my comment |t4_ndef_apdu|.

In general would it make sense to have a more elaborate routine for parsing here? I'd first look at the initial 2 bytes to get the instruction and afterwards look at the tail for the details.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dimi,

Thanks for the patch set. My knowledge about these tags is very limited. So let's discuss the issues at the work week. Do you have a test case?

Comment 6

[Dimi Lee [:dimi][:dlee]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Hi Dimi,
> 
> Thanks for the patch set. My knowledge about these tags is very limited. So
> let's discuss the issues at the work week. Do you have a test case?

Sure, let's discuss this in the work week.
Currently test cases are broken and i am fixing it, i will upload test case once the issue is fixed.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8442667 [details] [diff] [review]
Part2. Process type 4 tag read ndef command v1

Review of attachment 8442667 [details] [diff] [review]:
-----------------------------------------------------------------

::: hw/nfc-tag.c
@@ +61,5 @@
> +/* [T4TOP] Table 16 */
> +static const uint8_t t4t_rb_apdu[2] = { 0x00, 0xb0 };
> +
> +/* [T4TOP] Table 19 */
> +static const uint8_t t4t_ndef_apdu[5] = { 0x00, 0xa4, 0x00, 0x0c, 0x02 };

According to discussion, having the length field hard-coded to two is correct.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8442666 [details] [diff] [review]
Part1. Add type4 tag as a remote endpoint v1

Review of attachment 8442666 [details] [diff] [review]:
-----------------------------------------------------------------

::: hw/nfc-tag.c
@@ +136,5 @@
> +set_t4t_data(struct nfc_tag* tag, const uint8_t* ndef_msg, ssize_t len)
> +{
> +    assert(tag);
> +    assert(ndef_msg);
> +    assert(len < sizeof(tag->t.t4.format.data));

According to discussion, only the check shall be improved, copying is correct.

Comment 9

[Dimi Lee [:dimi][:dlee]]

Attachment: bug_1027479.patch

Comment 10

[Dimi Lee [:dimi][:dlee]]

Attachment: pull request for mozilla-b2g/platform_external_qemu

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8444343 [details] [diff] [review]
bug_1027479.patch

Review of attachment 8444343 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_tag.js
@@ +8,5 @@
>  
>  // TODO : Get this from emulator console command.
>  const T1T_RE_INDEX = 2;
>  const T2T_RE_INDEX = 3;
> +const T3T_RE_INDEX = 4;

Was this constant missing from the last test case?

Comment 12

[Dimi Lee [:dimi][:dlee]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8444343 [details] [diff] [review]
> bug_1027479.patch
> 
> Review of attachment 8444343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/tests/marionette/test_nfc_tag.js
> @@ +8,5 @@
> >  
> >  // TODO : Get this from emulator console command.
> >  const T1T_RE_INDEX = 2;
> >  const T2T_RE_INDEX = 3;
> > +const T3T_RE_INDEX = 4;
> 
> Was this constant missing from the last test case?

Yes.

Comment 13

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/platform_external_qemu/commit/bfbb47844a20625d9bb6c9212fa554d7808bd58e

but we still need to checkin the testcase right ?

Comment 14

[Dimi Lee [:dimi][:dlee]]

(In reply to Carsten Book [:Tomcat] from comment #13)
> master:
> https://github.com/mozilla-b2g/platform_external_qemu/commit/
> bfbb47844a20625d9bb6c9212fa554d7808bd58e
> 
> but we still need to checkin the testcase right ?

Yes, we also need to merge the testcase

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #14)
> (In reply to Carsten Book [:Tomcat] from comment #13)
> > master:
> > https://github.com/mozilla-b2g/platform_external_qemu/commit/
> > bfbb47844a20625d9bb6c9212fa554d7808bd58e
> > 
> > but we still need to checkin the testcase right ?
> 
> Yes, we also need to merge the testcase

could you generate that pull request ?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Carsten Book [:Tomcat] from comment #15)
> could you generate that pull request ?

It's a Gecko test. That said, it has conflicts. Looks like this patch depends on a testUrlT3TDiscover test which isn't landed on b-i at the moment.

Comment 17

[Dimi Lee [:dimi][:dlee]]

Attachment: Testcase patch v2

Comment 18

[Dimi Lee [:dimi][:dlee]]

Rebase testcase patch

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ca6ac5aa6b62

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca6ac5aa6b62