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

NFC Magic: New functions for Gen4 cards (DW to block 0/shadow mode) #98

Merged
merged 34 commits into from
Apr 22, 2024

Conversation

Leptopt1los
Copy link
Contributor

@Leptopt1los Leptopt1los commented Dec 30, 2023

What's new

  • New function: Set Gen4 card shadow mode
  • New function: Set Gen4 card direct write to block 0 mode (Gen2 behavour)
  • Get revision, get config moved into main Gen4 menu
  • Fixed: back button did not allow to exit from some scenes while the card is next to the Flipper HF RFID antenna

Verification

Shadow mode verification

  • build fap
  • write any 1k dump to gen4 card
  • edit dump sector data
  • run "check magic tag" on gen4 card
  • test all shadow modes (described here) by "write to initial card" from changed dump menu

Direct write to block 0 verification

  • set direct write to block 0 mode to Activated
  • try to write block 0 data via proxmark3 or Mifare Classic Tools (block 0 data must be written)
  • set direct write to block 0 mode to Deactivated
  • try to write block 0 data via proxmark3 or Mifare Classic Tools (block 0 data must not be written)

Back button event handlers verification

  • run "check magic tag" on any card
  • after new scene press back button. previous scene must be triggered

Cleanup verification

  • check get revision, get config, write functions. must works as in 1.4 release

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

@Leptopt1los Leptopt1los marked this pull request as draft January 1, 2024 09:27
@Leptopt1los Leptopt1los changed the title NFC Magic: Gen4 shadow mode management support NFC Magic: New functions for Gen4 cards (DW to block 0/shadow mode) Jan 1, 2024
@Leptopt1los Leptopt1los marked this pull request as ready for review January 2, 2024 10:11
@skotopes
Copy link
Member

skotopes commented Jan 3, 2024

@Leptopt1los what will be the difference between set standard config and wipe?

@Leptopt1los
Copy link
Contributor Author

@Leptopt1los what will be the difference between set standard config and wipe?

set standart config does not change the data on the card, but only the config. wipe can work out with an error for 06A0, and even be the cause of her "coma" itself. using set stabdart config you do not lose the current data on the card

if I understood you correctly, are you thinking about deleting the set default config item and transferring wipe from gen4_menu to gen4_actions? this probably made the code cleaner to some extent, but I can't say with full confidence that the wipe function can fully replace the set default config function

first, this scenario needs to be tested - will wipe be able to revive 06A0 in a coma (since in its normal state, wipe fails for it at the stage of writing standard data if the card lies exactly under the flipper)
secondly, a hypothetical user case: my gen4 contains important data for me, but she fell into a coma. if I do a wipe, I will lose this data. but if I do set standart config, the data on the card will be saved and it will come to life

@skotopes
Copy link
Member

skotopes commented Jan 3, 2024

Okay, I'll discuss it with NFC team and then merge.

skotopes
skotopes previously approved these changes Jan 20, 2024
@skotopes
Copy link
Member

@Leptopt1los config read is not working for me. My revision 06 A0. Both cards I got behave that way.

@skotopes
Copy link
Member

@gornekich can you take a look on this PR?

@gornekich
Copy link
Member

Hi @Leptopt1los
We came up with this design https://www.figma.com/file/18xDclT01Y07hA4mdbOcAA/Zero.-User-Flow-(Public)?type=whiteboard&node-id=0-1&t=Or8zkOl7bGi4V3lI-0 . This is quite big task to implement this design. I think the best way is to merge this PR first and then continue refactoring UI.

Regarding this PR, we need to fix navigation between the scenes. After successful action in gen4 menu we should return back to gen4 menu, not to the start screen. After it's fixed, me can merge the PR.

Please, let me know your thoughts about the design. Also, are you interested in implemented it? If not, we will do it ourselves.

@Leptopt1los Leptopt1los marked this pull request as draft February 20, 2024 18:03
@Leptopt1los
Copy link
Contributor Author

hi @gornekich!
I made one of the points of the redesign - I removed get config and get revision. created a handler for getInfo that requests the config and revision

questions:

  1. I would like a critical review of this commit. I'm not at all sure I did it well
    1.1 is it normal that my gen4_poller_get_info_handler returns two values ​​at once? and since they are returned in Gen4PollerEventData gen4_event_data (this is a union); I had to solder an anonymous structure
    ‘’’
    struct {
    uint8_t config_data[32];
    uint8_t revision_data[5];
    };
    ‘’’
    1.2 is this amount of logic normal in the nfc_magic_scene_gen4_show_info scene?
    1.3 are there any other potential problems, including architectural ones?
  2. case: we detected gen4. After the detection, we saw on the screen that this is gen4, its revision and some information that is taken from the config (for example, what type of card it is configured for). we write a different type of card on it. go to info - old data is there

this is how it will work according to the user flow from figma

To prevent this from happening, I added a GetInfo scene before Info, which again accesses the map. Will this solution suit us?
3. How can we display the revision and data from the config on the screen of the detected gen4? Do you need to change the detection function so that under the hood it also makes a request for info and returns them to the scene? Or should the insert be made already in the scene - AFTER the detection, knock on the door of the GetInfo handler?
4. The detector itself in the implementation requests the card config and checks its length. Maybe config can be returned from there?
5. Both on the Info screen and on the screen after the gen4 detection it is necessary to display what the card is currently configured for. this will be done by the FSM function, which will process the config. Where can this function be placed so that it can be called from these two stages?

@gornekich
Copy link
Member

gornekich commented Feb 29, 2024

@Leptopt1los I think it's time to make a separate class Gen4, which will hold just gen4 related data. It includes configuration, revision and parsed configuration data. I suggest making Gen4 fields public just to avoid writing too many getters/setters. The interface for this class could be something like this:

#pragma once

#include <stdint.h>

#define GEN4_CONFIG_MAX_SIZE (30)
#define GEN4_REVISION_MAX_SIZE (5)

#define GEN4_PASSWORD_LEN (4)
#define GEN4_ATS_LEN (17)
#define GEN4_ATQA_LEN (2)

typedef union {
    uint8_t data_raw[GEN4_CONFIG_MAX_SIZE];
    struct {
        uint8_t mf_type;
        uint8_t uid_len_code;
        uint8_t password[GEN4_PASSWORD_LEN];
        uint8_t gtu_mode;
        uint8_t ats[GEN4_ATS_LEN];
        uint8_t atqa[GEN4_ATQA_LEN];
        uint8_t sak;
        uint8_t mfu_mode;
        uint8_t total_blocks;
        uint8_t direct_write_mode;
    } data_parsed;
} Gen4Config;

typedef struct {
    uint8_t data[GEN4_REVISION_MAX_SIZE];
} Gen4Revision;

typedef struct {
    Gen4Config config;
    Gen4Revision revision;
} Gen4;

Gen4* gen4_alloc();

void gen4_free(Gen4* instance);

void gen4_reset(Gen4* instance);

void gen4_copy(Gen4* instance, const Gen4* other);

It would be nice to rework gen4_poller_get_config() and gen4_poller_get_revision() functions to work with Gen4Revision and Gen4Config types. Making new type for a data structure is zero cost operation, but it makes code more readable and helps you to avoid mistakes in future.

Further, Gen4Poller instance should allocate Gen4 instance and keep it up to date. I think the best option to achieve that is to make "mandatory" state for each poller mode which will read gen4 config and store it in Gen4 instance inside poller instance. I can see that you already added gen4_poller_get_current_cfg_handler(), I suggest to call it after RequestMode state.

Each time we want to change config in poller, we should follow these steps:

  1. Allocate new Gen4 instance and set desirable config.
  2. Call functions from gen4_poller_i.h to modify config on gen4 tag.
  3. If there is no error, copy Gen4 instance to poller's Gen4 instance and free first one.

After that's done, Gen4Poller interface should have function:
const Gen4* gen4_poller_get_data(Gen4Poller* instance);
The nfc_magic application should also have allocated Gen4 instance and update it each time right before calling gen4_poller_free(). This allows us to have up to date Gen4 instance on application level and correctly display all data.

Now answering your questions:

  1. With the approach I suggested there is no need in passing config and revision data through events. You will always have correct Gen4 data on application level. Also you can move gen4_get_shadow_mode_name(), gen4_get_direct_write_mode_name(), gen4_get_uid_len() function to Gen4 class and make it part of gen4 library.
  2. The approach with updating Gen4 instance helps with storing inconsistent data.
  3. Since gen4_poller_detect() signature is already differs from gen1a_poller_detect() there is no harm in passing Gen4 instance in this function. New signature could be: Gen4PollerError gen4_poller_detect(Nfc* nfc, uint32_t password, Gen4* gen4_data); where you can pass Gen4 instance from magic application for initial fill.
  4. See 3.
  5. I think making Gen4Config union to access raw and parsed data will make it unnecessary writing new "converting" function from raw to parsed data representation. All further parsing should be public function of Gen4 class.

@Leptopt1los
Copy link
Contributor Author

@gornekich can you take a look please?

@Leptopt1los
Copy link
Contributor Author

I also have several identical scenes that perform a similar action: nfc_magic_scene_gen4_set_default_cfg, nfc_magic_scene_gen4_set_direct_write_block_0_mode, nfc_magic_scene_gen4_set_shd_mode, nfc_magic_scene_gen4_get_info. Should I combine these scenes into one and select the desired action based on the state of the scene?

Copy link
Member

@gornekich gornekich left a comment

Choose a reason for hiding this comment

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

Good progress, thank you for working on that!
What do you think about jumping to Gen4PollerStateGetInfo state always before Gen4PollerStateSuccess state? I mean we can update internal Gen4 data right before poller finishes its work for all modes.

nfc_magic/lib/magic/protocols/gen4/gen4.h Outdated Show resolved Hide resolved
nfc_magic/lib/magic/protocols/gen4/gen4.h Outdated Show resolved Hide resolved
#include "core/check.h"

Gen4* gen4_alloc() {
Gen4* instance = (Gen4*)malloc(sizeof(Gen4));
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to cast (Gen4*) explicitly

nfc_magic/lib/magic/protocols/gen4/gen4.c Outdated Show resolved Hide resolved
nfc_magic/lib/magic/protocols/gen4/gen4.h Outdated Show resolved Hide resolved
nfc_magic/lib/magic/protocols/gen4/gen4_poller.c Outdated Show resolved Hide resolved
nfc_magic/lib/magic/nfc_magic_scanner.c Outdated Show resolved Hide resolved
Comment on lines 2 to 4
//TODO: INCAPSULATE?
#include "gui/scene_manager.h"
#include "protocols/gen4/gen4_poller_i.h"
Copy link
Member

Choose a reason for hiding this comment

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

"scene_manager.h" is already included in #include "../nfc_magic_app_i.h"

Copy link
Member

Choose a reason for hiding this comment

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

Don't include private API. There should be function in gen4_poller.h
const Gen4* gen4_poller_get_data(Gen4Poller* instance);

nfc_magic/scenes/nfc_magic_scene_gen4_get_info.c Outdated Show resolved Hide resolved
nfc_magic/scenes/nfc_magic_scene_gen4_select_shd_mode.c Outdated Show resolved Hide resolved
@Leptopt1los Leptopt1los marked this pull request as ready for review April 12, 2024 19:11
@Leptopt1los Leptopt1los requested a review from gornekich April 12, 2024 19:11
@gornekich
Copy link
Member

Well done, thanks a lot!
Last question to discuss - I think it would be better to update gen4 data on poller exit for all modes. If we do so - we won't need to explicitly read config one more time when we press Info submenu in Gen4 menu. We already have consistent Gen4 data in application and display it immediately

@gornekich gornekich merged commit 922a87c into flipperdevices:dev Apr 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants