Implement AT$NVM, AT$APKACCESS, and AT$DISUART #117

disk91 · 2022-10-27T14:52:55Z

Use AT$NVM=0,123 to store value 123 in user space 0
+OK=1,123
Use AT$NVM=0 to read value previously stored
+OK=123

It currently supports 4 user spaces (0 to 3) and values are 8 bits unsigned

Use AT$NVM=0,123 to store value 123 in user space 0 +OK=1,123 Use AT$NVM=0 to read value previously stored +OK=123 It currently supports 4 user spaces (0 to 3) and values are 8 bits unsigned

The AppKey is a secret you should not give an easy access to. I've added a command to avoid this access. Once locked it's not possible to regain access to the appkey. It can still write but not read. The additional command is AT$APKACCESS without any parameter.

disk91 · 2022-11-01T18:50:16Z

Additionnal command AT$APKACCESS added to protect AppKey against being read from the serial line.

…1310 MKR1310 use same wires for SPI Flash control and UART line. They are in conflict and SPI can't be use until the serial line has been disabled. This new command $DISUART manage this problem. The initial UART configuration is restored by forcing the PB12 line to 0 corresponding to LORA_IRQ_DUMB signal on MKR1310

janakj · 2022-11-05T22:24:58Z

Thank you very much for the PR!

I am looking at the AT command syntax and I am wondering if it might make more sense to use the action AT command syntax with parameters separated by a whitespace rather than the getter/setter syntax. The problem with the getter/setter syntax is that getters don't really support parameters, i.e., one cannot specify which memory location to read from. You found a clever workaround with AT$NVM=0, i.e., using a setter as a getter. But I worry this syntax might be a bit confusing for others and it also does not match the documentation.

How about this. To set the value at memory location 0 one would use:

AT$NVM 0,123
+OK

To obtain the current value at memory location 0, you would provide only one argument to the AT command:

AT$NVM 0
+OK=123

This should be a fairly trivial modification. You would need to call the handler function from the second field in atci_command_t and dispatch within the function based on how many parameters you receive. Take a look at the join handler, for example. That handler implements the same algorithm.

janakj

Thank you very much for the contribution! This PR seems to include three different features: (1) the user NVM stuff; (2) AppKey readout prevention; (3) some modification for MKRWAN1310 that I don't fully understand.

I am happy to merge (1), the user NVM feature. I made a few comments and suggestions on that code, but nothing big.

Regarding the other two features, could you, please, remove those from this PR and maybe start a new PR for each? I am not ready to merge (2) and (3) yet and having them on the same PR with (1) prevents me from merging (1) as well.

If you could start separate PRs for (2) and (3), we can then tend to those separately.

src/cmd.c

src/nvm.c

src/nvm.h

janakj · 2022-11-05T23:48:27Z

src/cmd.c

@@ -2147,6 +2157,7 @@ static const atci_command_t cmds[] = {
    {"$CW",          cw,      NULL,             NULL,             NULL, "Start continuous carrier wave transmission"},
    {"$CM",          cm,      NULL,             NULL,             NULL, "Start continuous modulated FSK transmission"},
    {"$NVM",         NULL,    set_nvm,          NULL,             NULL, "Store / Read data from Non Volatile Memory"},
+    {"$APKACCESS",   protect_appkey, NULL,      NULL,             NULL, "Protect AppKey against read access"},


This is an interesting idea, but as currently implemented this would only provide minimal (accidental) protection. As long as you have access to USART1 or USART2 on the Type ABZ module, which all devices that I have seen have to support firmware updates, you can always access any of the keys stored in EEPROM.

One way to do it is to flash a modified version of the firmware into the modem, or even just an older version of this firmware. Another method is to switch the STM32 inside Type ABZ into a bootloader mode and then you can read the EEPROM from there. This firmware currently does not lock down the Type ABZ module in any way, so these access methods are always possible.

Also, the AppKey is only used in the OTAA mode to derive the AppSKey. In the APB mode, the modem directly uses the AppSKey. Since we support both modes in the firmware, we would need to lock both keys.

It might be a good idea to think about how we could provide more protection for the various security keys, but perhaps we should do that in a separate PR or discussion thread. If you don't mind, I would like to keep this PR for the user NVM feature.

I agree, this should also applied to AppSkey and NetSkey

If that's not secret, what's your use case for AT$APKACCESS? Did you have any specific attacks or threats in mind?

The idea is to protect the keys and make access to the most private one "AppKey" as much complex as possible. As the modem is saving it into the eeprom you don't need to store it in the MCU it is really great, it can be removed from the MCU firmware with a dynamic loading, perfect.
But it's really easy to send an AT command direectly on the modem line from an external system to extract it. So if the AT command can be blocked, the only way is to attack the eeprom, this is more complex. To go to the next step, you can take a look at my software secure store for keys in my abz sdk on my github.
Here I don't want to implement a such security level but it's just to avoid a so simple way to extract keys.

Honestly, i'm not really think about a real protection way with this command, but it's to avoid all the "security researchers" and other script kiddies mocking on IoT because they discovered how to send a bit on a serial line wire... so it's "best practice" to make a such minimal protection on credentials.

Makefile

janakj · 2022-11-05T23:59:49Z

src/cmd.c

+#if MKR1310 == 1
+// The MKR1310 is using the same wires for SPI and UART
+// To be able to use the embedded SPI Flash, we need to switch UART Off
+// As uart is used for waking the device up when going into sleep mode
+// we will disable the sleep mode too during this period of time
+// Uart will be resumed by setting GPIO B12 to LOW (it has a pull-up on the line)
+static uint8_t __prev_sleep;
+static uint8_t __uart_disable = 0;
+static void disable_uart(atci_param_t *param) {
+    // simpler to add everything in a single place, 
+    // make sure these lines are INPUT to not conflict with external signals from SPI
+    GPIO_InitTypeDef cfg = {
+        .Mode = GPIO_MODE_INPUT,
+        .Pull = GPIO_NOPULL
+    };
+    gpio_init(GPIOB, GPIO_PIN_12, &cfg);
+    gpio_init(GPIOB, GPIO_PIN_13, &cfg);
+    gpio_init(GPIOB, GPIO_PIN_14, &cfg);
+    gpio_init(GPIOB, GPIO_PIN_15, &cfg);
+
+    // No need to deactivate uart if the wakeup signal is true
+    int v = gpio_read(GPIOB, GPIO_PIN_12);
+    if ( v == 0 ) abort(ERR_PARAM);
+    OK_();
+
+    lpuart_disable();
+    __prev_sleep = sysconf.sleep;
+    __uart_disable = 1;
+    sysconf.sleep = 0; // prevent the device to go deep sleep and never waked up due to serial line deactivation
+}
+
+// this is monitoring the PB12 line and restart the UART when to comes low
+void process_uart_wakeup(void) {
+    // nothing to do
+    if ( __uart_disable == 0 ) return;
+
+    int v = gpio_read(GPIOB, GPIO_PIN_12);
+    if ( v == 0 ) {
+        // restart is requested
+        lpuart_enable();
+        sysconf.sleep = __prev_sleep;
+        __uart_disable = 0;
+        OK_();
+    }
+}
+
+#endif


I think I may need some explanation or rational for this entire change. Disabling LUPART this way and preventing the sleep mode sounds dangerous to me. The LPUART port uses a combination of DMA and IRQ wakeups to keep power consumption low.

I am not familiar with MRKWAN1310 boards. If this is indeed needed, I would be happy to consider including the functionality in some way, but I would like to understand the problem a little better first.

Would you mind removing this commit from this PR and starting a new PR or discussion thread instead?

This is really specific to MKR1310 because the same wires are used for ABZ UART and Flash SPI. As a consequence you can't use the onboarded flash when the UART is activ.
I did remove sleep when uart is disabled because only the UART wakes the ABZ up when going to sleep, so if UART is down .. that was a lower impact on the code than changing the sleep functions to wakeup on a GPIO. You should use the flash on short duration so it have no impact on overall energy consumption.
This is really a bypass for a bug of coneption on MKR1310

I checked the MKRWAN1310 schematic and I see what the problem is now.

Do you prefer your solution (disabling LPUART temporarily), or should we attempt something else? Would it make sense, for example, to design a SPI-based interface as an alternative to the AT command interface? I am not that worried about the amount of additional code and an alternative to the ATCI would seem generally useful.

If you don't mind, please spin this feature off into a separate PR and we can discuss it there.

I should have made different branches for the PR but unfortunatelly thay are on master, so I can dissociate them. But I think you can accept to merge commit by commit and the commit are for the different features. So you can delay the commit related to the UART / SPI.

Ok. I can cherry-pick the NVM-related commits, but I am not sure if Github preserves all authorship and context information that way. We will see.

I don't think making a SPI interface is a good idea as it changes the Arduino lib, even if rewritting it would be a good idea as it is badly written, It will not be adopted by the community, they won't want to port their code to a new lib if they use the default one. So my commit is the best solution I found against many worst them. I think we should keep that one.

Fair enough. I brought this up since there appears to be an effort to design MKRWAN library 2.0 and a potential SPI interface was mentioned there. I figured this might be an opportunity to get something good out of a bad situation.

Anyway, let me take a look at your approach. That'll be much quicker to finish. I wonder if we should do something about the busy waiting while LPUART is disconnected, e.g., put the LoRa module to sleep as usual and configure a GPIO pin to generate an interrupt to wake it up. What do you think?

Oops. It looks like I edited your comment instead of posting a reply. Sorry! Github UI mixup, I guess.

unclear on what you mean ?

using GPIO wake up would be good as this is also the way I use to restore the lpuart.
I prefer you make this modif as I know (for having implemented a lot around) that is is sensitive to touch the low-power code and can have side impact on functions I did not touched in my modifications

disk91 · 2022-11-06T10:45:20Z

Thank you for you detailed review, I'll made the change soon.

disk91 · 2022-11-07T22:16:53Z

The new commit should fix all the comments made on $NVM

disk91 · 2022-11-07T22:30:51Z

Last commit to rollback modification on Makefile and fix a compilation warning

janakj · 2022-11-13T18:13:14Z

@disk91 I have merged all AT$NVM-related code and documented the new AT command in the wiki. Please feel free to edit!

disk91 · 2022-11-13T18:17:36Z

@janakj thank you !

janakj · 2023-02-12T15:54:08Z

Hi @disk91, I am working on merging your AT$DISUART pull request. I am thinking of releasing version 1.3.0 before the end of February with this improvement included. I would like to include a slightly enhanced version of this code which sleeps while the LPUART1 port is detached and which will still be able to receive downlinks in class C operational mode (requires downstream data buffering). I had one question about your original code.

You are using PB12 to signal to the modem to reattach the port. Since we have to use a GPIO pin for this purpose, I am wondering if we really need the AT command AT$DISUART? Would it not be simpler and cleaner to entirely control when LPUART1's GPIO pins are attached/detached via the PB12 GPIO pin? For example, to detach, the host would pull PB12 down to 0 and wait for, e.g., '+EVENT=0,9' from the modem. The event would signal that the modem detached the LPUART1 GPIOs. Similarly, to re-attach the GPIOs, the host would let PB12 go up to 1 and wait for another event, e.g., '+EVENT=0,10'.

What do you think?

-Jan

disk91 · 2023-02-17T12:27:31Z

Hi @disk91, I am working on merging your AT$DISUART pull request. I am thinking of releasing version 1.3.0 before the end of February with this improvement included. I would like to include a slightly enhanced version of this code which sleeps while the LPUART1 port is detached and which will still be able to receive downlinks in class C operational mode (requires downstream data buffering). I had one question about your original code.

You are using PB12 to signal to the modem to reattach the port. Since we have to use a GPIO pin for this purpose, I am wondering if we really need the AT command AT$DISUART? Would it not be simpler and cleaner to entirely control when LPUART1's GPIO pins are attached/detached via the PB12 GPIO pin? For example, to detach, the host would pull PB12 down to 0 and wait for, e.g., '+EVENT=0,9' from the modem. The event would signal that the modem detached the LPUART1 GPIOs. Similarly, to re-attach the GPIOs, the host would let PB12 go up to 1 and wait for another event, e.g., '+EVENT=0,10'.

What do you think?

-Jan

Hi Jan,
Sorry for the late answer, The reason why I liked the idea of using an AT command to switch that mode was to make sure that it will not impact the standard behavior. We don't know how the PB12 is configured on the Arduino side and people will understand why this needs to be setup. As a consequence I was prefering an AT command so whatever is the arduino pin configuration it will have no impact.
Paul

janakj · 2023-02-19T03:20:12Z

Hi @disk91, I am working on merging your AT$DISUART pull request. I am thinking of releasing version 1.3.0 before the end of February with this improvement included. I would like to include a slightly enhanced version of this code which sleeps while the LPUART1 port is detached and which will still be able to receive downlinks in class C operational mode (requires downstream data buffering). I had one question about your original code.
You are using PB12 to signal to the modem to reattach the port. Since we have to use a GPIO pin for this purpose, I am wondering if we really need the AT command AT$DISUART? Would it not be simpler and cleaner to entirely control when LPUART1's GPIO pins are attached/detached via the PB12 GPIO pin? For example, to detach, the host would pull PB12 down to 0 and wait for, e.g., '+EVENT=0,9' from the modem. The event would signal that the modem detached the LPUART1 GPIOs. Similarly, to re-attach the GPIOs, the host would let PB12 go up to 1 and wait for another event, e.g., '+EVENT=0,10'.
What do you think?
-Jan

Hi Jan, Sorry for the late answer, The reason why I liked the idea of using an AT command to switch that mode was to make sure that it will not impact the standard behavior. We don't know how the PB12 is configured on the Arduino side and people will understand why this needs to be setup. As a consequence I was prefering an AT command so whatever is the arduino pin configuration it will have no impact. Paul

Hi Paul,

That makes a lot of sense. Thanks for the explanation. I will merge a version where LPUART1 can be detached through the AT command then.

-Jan

janakj · 2023-03-09T03:40:34Z

Hi Paul,

I just merged the last of the three features from this PR, the AT command AT$DETACH which can be used to detach the ATCI port. The merged version is heavily modified. I wanted to make sure the modem enters a sleep mode while the port is detach and that we don't lose any incoming data while the port is detach. This required some changes to the internal lpuart port subsystem. I tested it and it seems to work. To re-attach the port, the host MCU must pull PB12 down.

Note: one needs to be careful when enabling the detachable LPUART feature in debugging mode. There is a potential for a GPIO conflict, so if you want to build the firmware in a debug mode for MKRWAN boards, you may need to set DEBUG_MCU to 0.

I also just released version 1.3.0 which has all the features from this PR. If you agree, we can close this PR.

-Jan

disk91 · 2023-03-09T06:24:38Z

Hi Jan !Thank you for releasing it !I have a new feature i would like to add, i will take a look at it : i needed a command to get the FCNT value for making end to end crypto like aes ctr. Paul_______________________________Paul Pinaulthttp://www.disk91.comhttp://www.twitter.com/disk_91Le 9 mars 2023 à 04:40, Jan Janak ***@***.***> a écrit : Hi Paul, I just merged the last of the three features from this PR, the AT command AT$DETACH which can be used to detach the ATCI port. The merged version is heavily modified. I wanted to make sure the modem enters a sleep mode while the port is detach and that we don't lose any incoming data while the port is detach. This required some changes to the internal lpuart port subsystem. I tested it and it seems to work. To re-attach the port, the host MCU must pull PB12 down. Note: one needs to be careful when enabling the detachable LPUART feature in debugging mode. There is a potential for a GPIO conflict, so if you want to build the firmware in a debug mode for MKRWAN boards, you may need to set DEBUG_MCU to 0. I also just released version 1.3.0 which has all the features from this PR. If you agree, we can close this PR. -Jan —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

janakj · 2023-03-09T21:41:04Z

Hi Paul,

If you need access to the frame counters, check out the AT+FRMCNT command.

If this is not what you need, please feel free to submit a separate PR.

-Jan

disk91 added 3 commits October 27, 2022 10:32

test commit

20d9c5e

Add $NVM command for supporting User Storage Space

fb8b5b8

Use AT$NVM=0,123 to store value 123 in user space 0 +OK=1,123 Use AT$NVM=0 to read value previously stored +OK=123 It currently supports 4 user spaces (0 to 3) and values are 8 bits unsigned

janakj requested changes Nov 6, 2022

View reviewed changes

Apply requested modification on AT$NVM command commit

e0b9525

fix some compilation issues and rollback Makefile modifications

4aabe2f

janakj mentioned this pull request Nov 13, 2022

Add AT command to disconnect LPUART #119

Closed

janakj changed the title ~~Addition of the $NVM command to support User Storage~~ Implement AT$NVM, AT$APKACCESS, and AT$DISUART Nov 13, 2022

janakj approved these changes Nov 13, 2022

View reviewed changes

janakj mentioned this pull request Nov 13, 2022

Protection for LoRaWAN security keys in EEPROM #121

Open

janakj closed this Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AT$NVM, AT$APKACCESS, and AT$DISUART #117

Implement AT$NVM, AT$APKACCESS, and AT$DISUART #117

disk91 commented Oct 27, 2022

disk91 commented Nov 1, 2022

janakj commented Nov 5, 2022

janakj left a comment

janakj Nov 5, 2022 •

edited

Loading

disk91 Nov 13, 2022

janakj Nov 15, 2022

disk91 Nov 16, 2022 •

edited

Loading

janakj Nov 5, 2022

disk91 Nov 6, 2022

janakj Nov 7, 2022

disk91 Nov 7, 2022 •

edited by janakj

Loading

janakj Nov 9, 2022

disk91 Nov 10, 2022

disk91 Nov 10, 2022

disk91 Nov 10, 2022

disk91 commented Nov 6, 2022

disk91 commented Nov 7, 2022

disk91 commented Nov 7, 2022

janakj commented Nov 13, 2022

disk91 commented Nov 13, 2022

janakj commented Feb 12, 2023 •

edited

Loading

disk91 commented Feb 17, 2023

janakj commented Feb 19, 2023

janakj commented Mar 9, 2023

disk91 commented Mar 9, 2023 via email

janakj commented Mar 9, 2023

Implement AT$NVM, AT$APKACCESS, and AT$DISUART #117

Implement AT$NVM, AT$APKACCESS, and AT$DISUART #117

Conversation

disk91 commented Oct 27, 2022

disk91 commented Nov 1, 2022

janakj commented Nov 5, 2022

janakj left a comment

Choose a reason for hiding this comment

janakj Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disk91 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disk91 Nov 7, 2022 • edited by janakj Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disk91 commented Nov 6, 2022

disk91 commented Nov 7, 2022

disk91 commented Nov 7, 2022

janakj commented Nov 13, 2022

disk91 commented Nov 13, 2022

janakj commented Feb 12, 2023 • edited Loading

disk91 commented Feb 17, 2023

janakj commented Feb 19, 2023

janakj commented Mar 9, 2023

disk91 commented Mar 9, 2023 via email

janakj commented Mar 9, 2023

janakj Nov 5, 2022 •

edited

Loading

disk91 Nov 16, 2022 •

edited

Loading

disk91 Nov 7, 2022 •

edited by janakj

Loading

janakj commented Feb 12, 2023 •

edited

Loading