8000 nrf52: support new access port protection mechanism on new nRF52 chips by bradjc · Pull Request #3422 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

nrf52: support new access port protection mechanism on new nRF52 chips #3422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 11, 2023
Merged

nrf52: support new access port protection mechanism on new nRF52 chips #3422

merged 8 commits into from
May 11, 2023

Conversation

bradjc
Copy link
Contributor
@bradjc bradjc commented Apr 14, 2023 •
edited
Loading

Pull Request Overview

It seems like recently nordic got bad press for having a vulnerability in how they disable jtag access, so newer versions of the nrf52840 change how they restrict jtag access, which essentially restricts jtag by default. This PR adds support to disable the protection so we can use and reprogram newer nRFs without re-flashing every time.

This requires setting both a "HW" register (aka in flash), and a "SW" register (aka in RAM) to 0x5a. The HW register can just be done once. The SW register needs to be done on every boot. Note, this is only for the new versions. So this code checks the variant and only includes it on the new variant.

Testing Strategy

Running tock on a new nrf52840dk board.

TODO or Help Wanted

Still need to set the UICR, or for now running nrfjprog --recover works.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@github-actions github-actions bot added the nrf Change pertains to the nRF5x family of MCUs. label Apr 14, 2023
twilfredo
twilfredo previously approved these changes Apr 17, 2023
View reviewed changes
brghena
brghena reviewed Apr 17, 2023
View reviewed changes
@bradjc
Copy link
Contributor Author
bradjc commented Apr 18, 2023

I added functionality to now set the approtect field in the UICR in the startup component.

I also refactored how we check for which variant in uicr.rs when disabling the approtect function.

I tested this PR on a new board and an old board (nrf52840dk) and both work.

brghena
brghena previously approved these changes Apr 18, 2023
View reviewed changes
twilfredo
twilfredo previously approved these changes Apr 19, 2023
View reviewed changes
@bradjc
Copy link
Contributor Author
bradjc commented Apr 20, 2023

What's wrong with netlify? Something in this PR?

@twilfredo
Copy link
Contributor
twilfredo commented Apr 21, 2023 •
edited
Loading

What's wrong with netlify? Something in this PR?

Yea the error is this URL is not a hyperlink, I think it wants the urls to be wrapped in <>

Log:

6:54:22 AM:  Documenting nrf52 v0.1.0 (/opt/build/repo/chips/nrf52)
6:54:22 AM: error: this URL is not a hyperlink
6:54:22 AM:  --> chips/nrf52/src/approtect.rs:7:5
6:54:22 AM:   |
6:54:22 AM: 7 | //! https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52840%2Fdif.html&cp=5_0_0_3_7_1&anchor=register.DISABLE
6:54:22 AM:   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<[https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52840%2Fdif.html&cp=5_0_0_3_7_1&anchor=register.DISABLE>](https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52840%2Fdif.html&cp=5_0_0_3_7_1&anchor=register.DISABLE%3E)`
6:54:22 AM:   |
6:54:22 AM:   = note: bare URLs are not automatically turned into clickable links
6:54:22 AM:   = note: `-D rustdoc::bare-urls` implied by `-D warnings`
6:54:22 AM: error: this URL is not a hyperlink
6:54:22 AM:   --> chips/nrf52/src/approtect.rs:70:11
6:54:22 AM:    |
6:54:22 AM: 70 |     /// - https://devzone.nordicsemi.com/f/nordic-q-a/96590/how-to-disable-approtect-permanently-dfu-is-needed
6:54:22 AM:    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<[https://devzone.nordicsemi.com/f/nordic-q-a/96590/how-to-disable-approtect-permanently-dfu-is-needed>](https://devzone.nordicsemi.com/f/nordic-q-a/96590/how-to-disable-approtect-permanently-dfu-is-needed%3E)`
6:54:22 AM:    |
6:54:22 AM:    = note: bare URLs are not automatically turned into clickable links
6:54:22 AM: error: this URL is not a hyperlink
6:54:23 AM:   --> chips/nrf52/src/approtect.rs:71:11
6:54:23 AM:    |
6:54:23 AM: 71 |     /// - https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/working-with-the-nrf52-series-improved-approtect
6:54:23 AM:    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<[https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/working-with-the-nrf52-series-improved-approtect>](https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/working-with-the-nrf52-series-improved-approtect%3E)`
6:54:23 AM:    |
6:54:23 AM:    = note: bare URLs are not automatically turned into clickable links
6:54:23 AM: error: could not document `nrf52`

twilfredo
twilfredo reviewed Apr 21, 2023
View reviewed changes
chips/nrf52/src/approtect.rs Outdated

//! Access port protection
//!
//! https://infocenter.nordicsemi.com/index.jsp?topic=%2Fps_nrf52840%2Fdif.html&cp=5_0_0_3_7_1&anchor=register.DISABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

here <url>

twilfredo
twilfredo reviewed Apr 21, 2023
View reviewed changes
chips/nrf52/src/approtect.rs Outdated
/// (software). For older variants this is just a no-op.
///
/// - https://devzone.nordicsemi.com/f/nordic-q-a/96590/how-to-disable-approtect-permanently-dfu-is-needed
/// - https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/working-with-the-nrf52-series-improved-approtect
Copy link
Contributor

Choose a reason for hiding this comment

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

these two here <url>

@bradjc
Copy link
Contributor Author
bradjc commented Apr 21, 2023

What? This tool can tell these are URLs so it can give us errors informing us it needs us to tell it these are URLs? What problem is this solving, rustdoc?

@twilfredo
Copy link
Contributor

What? This tool can tell these are URLs so it can give us errors informing us it needs us to tell it these are URLs? What problem is this solving, rustdoc?

I think this is just a lint being treated as an error, see bare_urls

@bradjc
rustdoc urls
19f107f 
apparently rustdoc notifies us that it has detected urls which we didn't
tell it are urls so we can tell it they are urls. thanks rustdoc.
@bradjc bradjc dismissed stale reviews from twilfredo and brghena via 19f107f April 26, 2023 14:24
twilfredo
twilfredo previously approved these changes Apr 27, 2023
View reviewed changes
brghena
brghena previously approved these changes Apr 28, 2023
View reviewed changes
ppannuto
ppannuto previously approved these changes Apr 28, 2023
View reviewed changes
@ppannuto ppannuto added the last-call Final review period for a pull request. label Apr 28, 2023
@lschuermann lschuermann dismissed stale reviews from ppannuto, brghena, and twilfredo via 2200560 May 2, 2023 03:15
lschuermann
lschuermann approved these changes May 2, 2023
View reviewed changes
Copy link
Member
@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! I'm yet to test it as I didn't have access to my (old) nRF52840DK.

I added a minor change which exhaustively lists all the chip variants for which the SwDisable switch is not required. This is to prevent the scenario where a new revision would be supported at first (because it matches on the Unspecified variant), and someone adds a definition for it to the ficr module, which would then implicitly break it in a non-obvious way due to the default match. Having the exhaustive list ensures that the compiler yells at us when this match diverges from the ficr::Variant enum definition.

@lschuermann lschuermann requested review from brghena and ppannuto May 2, 2023 03:20
@bradjc
Copy link
Contributor Author
bradjc commented May 11, 2023

Ready?

@brghena
Copy link
Contributor
brghena commented May 11, 2023

bors r+

@bors
Copy link
Contributor
bors bot commented May 11, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9f2e2f6 into master May 11, 2023
@bors bors bot deleted the nrf52-new-variant branch May 11, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brghena brghena brghena approved these changes

@lschuermann lschuermann lschuermann approved these changes

@twilfredo twilfredo twilfredo left review comments

@ppannuto ppannuto Awaiting requested review from ppannuto

Assignees
No one assigned
Labels
last-call Final review period for a pull request. nrf Change pertains to the nRF5x family of MCUs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bradjc 358C @twilfredo @brghena @ppannuto @lschuermann
0