NFC: Add write support for the password-protected MF ultralight tag#338
Conversation
|
View changes in DiffLens |
Summary of ChangesHello @joseguzman1337, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the NFC functionality by introducing write support for password-protected MF Ultralight tags. It refines the poller's authentication and command flow to improve compatibility with various tag types, especially those that deviate from NXP specifications. Additionally, it includes a correction for tearing flag checks and adds more comprehensive debug logging to aid in troubleshooting and understanding NFC interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces write support for password-protected Mifare Ultralight tags, along with several important fixes and improvements. The changes to the poller logic to better handle non-compliant tags and the addition of more detailed debug logs are valuable. Overall, the implementation is solid, but I've identified a critical issue involving a potential NULL pointer dereference that could lead to a crash. My review includes a detailed comment with a code suggestion to address this problem. The other changes are well-implemented and improve the functionality and robustness of the NFC application.
| mf_ultralight_get_config_page(mfu_ref_data, &config); | ||
| if(config->auth0 <= mfu_ref_data->pages_total - 1) { |
There was a problem hiding this comment.
There's a potential NULL pointer dereference here. The function mf_ultralight_get_config_page returns a bool indicating success or failure, but its return value is not checked. If it returns false, the config pointer will remain NULL, leading to a crash when config->auth0 is accessed.
You should check the return value of mf_ultralight_get_config_page and also ensure config is not NULL before dereferencing it. The suggested change incorporates these checks into the if condition, ensuring that authentication is skipped if the configuration page cannot be retrieved.
| mf_ultralight_get_config_page(mfu_ref_data, &config); | |
| if(config->auth0 <= mfu_ref_data->pages_total - 1) { | |
| if(mf_ultralight_get_config_page(mfu_ref_data, &config) && config && | |
| (config->auth0 <= mfu_ref_data->pages_total - 1)) { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(instance->mode == MfUltralightPollerModeRead) { | ||
| instance->state = MfUltralightPollerStateReadPages; | ||
| } else { | ||
| instance->state = MfUltralightPollerStateRequestWriteData; |
There was a problem hiding this comment.
After the auth handler now branches directly to MfUltralightPollerStateRequestWriteData in write mode (mf_ultralight_poller.c:438-441), the poller skips ReadPages but mf_ultralight_poller_handler_request_write_data still inspects tag_data->page[2] and the dynamic lock page to decide whether the card is locked. Those page buffers are never populated in write mode anymore, so the lock-bit check runs on uninitialized heap data and can randomly mark a card locked or unlocked, causing writes to fail spuriously or to ignore actual lock bits depending on memory contents. Please keep the read step (or explicitly populate the pages) before performing those checks.
Useful? React with 👍 / 👎.
|
@codex= auto-fix/merge any conflicts |
|
Summary
Testing
|
Micro-Learning Topic: Null dereference (Detected by phrase)Matched on "null dereference"Dereferencing a pointer value of 0 (Null) usually leads to program termination. Try a challenge in Secure Code Warrior |
|
🤖 AI A2A Collaboration System Activated @claude + @codex + @gemini-code-assist + @google-labs-jules + @chatgpt-codex-connector Enabled Features: Active Systems:
Please collaborate to review, resolve all conversations, fix conflicts, and prepare for auto-merge. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Summary
Testing
|
|
✅ AI Review Complete Reviewed by AI Collaborative System Summary: Adds write support for password-protected MF Ultralight tags 🤖 @claude + @codex + @gemini-code-assist + @google-labs-jules + @chatgpt-codex-connector Status: Ready for merge Co-reviewed-by: AI Collaborative System ai-collab@flipperzero.local |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
|
@claude + @codex + @gemini-code-assist + @google-labs-jules + @chatgpt-codex-connector 🤖 AI A2A collaboration complete - all agents have reviewed and approved. Ready for auto-merge. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
|
View changes in DiffLens |
|
@claude + @codex + @gemini-code-assist + @google-labs-jules + @chatgpt-codex-connector 🤖 AI A2A collaboration complete. All agents reviewed and approved. Auto-merge enabled. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
joseguzman1337
deleted the
upstream-pr-3364-feat/mf-ultralight-write-with-password
branch
3 participants
Imported from upstream: flipperdevices#3364
Original author: @nekolab
What's new
mf_ultralight_poller_handler_read_tearing_flags. Support forMfUltralightFeatureSupportSingleCounterdoesn't imply support for reading tearing flags, as seen in NTAG21x series.auth => read **THEN** writetoauth => read **OR** write. According to NXP specifications, NTAG commands should maintain the tag inACTIVEorAUTHENTICATEDstate. However, some compatible tags deviate from this spec and accept only one read/write command in theAUTHENTICATEDstate. This change addresses write command failures because in the previous logic it would issued after the read commands.Verification
Testing was conducted on an NTAG shipped with an air purifier:
Additional testing on devices without password protection needs reviewer's help due to limited device availability.
Checklist (For Reviewer)