Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API to enforce ISO15693 mode #3885

Closed
wants to merge 8 commits into from
Closed

Conversation

aaronjamt
Copy link

@aaronjamt aaronjamt commented Sep 8, 2024

What's new

  • Adds a new API to the NFC parser to allow applications to enforce a specific parsing mode. Some apps, such as Picopass, will always use one of the modes, so disabling autodetection can help to prevent issues resulting from incorrectly autodetecting the state (i.e. due to noise). It also includes a way to re-enable autodetection, should that be useful in the future.

Verification

  • Build a modified version of the Picopass app which calls the new nfc_iso15693_force_1outof4 method and emulate a card. Scanning the card on the reader no longer causes the Flipper to crash when the reader has LF enabled.

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

@nvx
Copy link
Contributor

nvx commented Sep 8, 2024

A bit of background, picopass only ever uses 1of4 modulation not 1of256. As readers will never use 1of256 if noise ever makes it appear like a 1of256 SOF was sent it will hang there indefinitely as the reader will never send a 1of256 EOF causing #3343

It is probably worth also having a timeout (we know the maximum frame size, if we don't see the rest of the frame in this time we should reset the emulation state ready for the next frame) and fix what looks like a buffer overrun in the 1of256 code (flipperdevices/flipperzero-good-faps#105), but even with those two things fixed I still think it makes sense allowing emulation code to opt out of 1of256 when it's known it should never happen as accidentally interpreting something as the 1of256 SOF would still likely interrupt emulation until the timeout occurs which would almost certainly cause the reader to time out.

A quick look at the patch seems fine to me, but I'm not an expert on the HAL design.

@aaronjamt aaronjamt marked this pull request as ready for review September 15, 2024 02:32
@bettse
Copy link
Contributor

bettse commented Sep 17, 2024

This should be an easier one to review since it is just adding a new API and not changing how anything functions

aaronjamt added a commit to aaronjamt/Momentum-Firmware that referenced this pull request Sep 17, 2024
Willy-JL added a commit to Next-Flip/Momentum-Firmware that referenced this pull request Sep 17, 2024
* Add API to enforce ISO15693 mode
See flipperdevices/flipperzero-firmware#3885

* Revert symbols version

* Format

* Update changelog

---------

Co-authored-by: Willy-JL <[email protected]>
@@ -26,6 +26,7 @@ typedef enum {
struct Iso15693Parser {
Iso15693ParserState state;
Iso15693ParserMode mode;
bool detect_mode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this detect_mode be moved to the Iso15693ParserMode enum?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so you're proposing to add two new modes like Iso15693ParserModeForced1OutOf4 and Iso15693ParserModeForced1OutOf256 in place of the boolean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but there is a more important thing, which I've missed previously. Putting protocol specific functions into nfc.c / nfc.h layer it's not very good idea, because nfc layer was designed to be a generic protocol free. I had a quick conversation with original nfc stack designer and he agreed on that.
Also, we came up to the idea that we need to investigate this problem deeper and try to fix this in iso15 parser. Currently I'm investigating this issue and will be in touch with you on the results.

@skotopes
Copy link
Member

Fixed in #3988

@skotopes skotopes closed this Feb 13, 2025
@nvx
Copy link
Contributor

nvx commented Feb 20, 2025

Fixed in #3988

While this fixed the crash bug in the picopass code, there are certain cards based on 15693 (eg, picopass) that does does not support the 1of256 mode so technically without this functionality it would always be possible to detect emulation vs a real card if the reader tested if the card responds to 1of256 mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants