Closed Bug 1027479 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S2 (23may)

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=3])

Attachments

(2 files, 3 obsolete files)

Emulator should support processing type 4 tag
- reading function should be implemented in this bug
Attachment #8442666 - Flags: review?(tzimmermann)
Attachment #8442667 - Flags: review?(tzimmermann)
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|?
Attachment #8442666 - Flags: review?(tzimmermann) → review-
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.
Attachment #8442667 - Flags: review?(tzimmermann) → review-
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?
(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 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.
Attachment #8442667 - Flags: review- → review+
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.
Attachment #8442666 - Flags: review- → review+
Attached patch bug_1027479.patch (obsolete) — Splinter Review
Attachment #8444343 - Flags: review?(tzimmermann)
Attachment #8442666 - Attachment is obsolete: true
Attachment #8442667 - Attachment is obsolete: true
Attachment #8444346 - Flags: review+
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?
Attachment #8444343 - Flags: review?(tzimmermann) → review+
(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.
Keywords: checkin-needed
(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
(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 ?
(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.
Keywords: checkin-needed
Attachment #8444343 - Attachment is obsolete: true
Attachment #8449196 - Flags: review+
Rebase testcase patch
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca6ac5aa6b62
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.