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.

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

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20d9c5e
test commit
disk91 Oct 27, 2022
fb8b5b8
Add $NVM command for supporting User Storage Space
disk91 Oct 27, 2022
1bd604a
Add a feature to block read access on appkey
disk91 Nov 1, 2022
58e31f9
Add ability to disconnect UART for being able to use SPI flash on MKR…
disk91 Nov 1, 2022
e0b9525
Apply requested modification on AT$NVM command commit
disk91 Nov 7, 2022
4aabe2f
fix some compilation issues and rollback Makefile modifications
disk91 Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,19 @@ DEBUG_PORT ?= 1
# also the default value. Hardwario LoRa devices use this pin.
#
# 2 - PB6 (pin 39). Arduino MKRWAN1310 boards use this pin.
TCXO_PIN ?= 1
TCXO_PIN ?= 2
janakj marked this conversation as resolved.
Show resolved Hide resolved

# Specific target MKR1310, add a feature to deactivate the serial port based on
# PB12 GPIO. This is because Arduino team used same pins for serial line and SPI
# lines. As a consequence, Serial line activity prevent to use the SPI port and the
# onboarded SerialFlash.
# The Arduino documentation recommands to maintain the modem in a reset mode but this
# will make the LoRaWAN session lost, so this approach is not working. With the ability
# to deactivate the serail line on request, this may solve the problem even if the right
# solution should be to redesign this board.
# 0 - Disable this specific feature
# 1 - Enable this specific feature with control line PB12 set to 0 (pull-up on the line)
MKR1310 ?= 1

################################################################################
# You shouldn't need to edit the text below under normal circumstances. #
Expand Down Expand Up @@ -306,7 +318,7 @@ CFLAGS += -DUSE_FULL_LL_DRIVER

CFLAGS += -DDEFAULT_UART_BAUDRATE=$(DEFAULT_UART_BAUDRATE)

CFLAGS += -DTCXO_PIN=$(TCXO_PIN)
CFLAGS += -DTCXO_PIN=$(TCXO_PIN) -DMKR1310=$(MKR1310)

# Extra flags to be only applied when we compile the souce files from the lib
# subdirectory. Since that sub-directory contains third-party code, disable some
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

The aim of this project is to develop open source LoRaWAN modem for the [Type ABZ](https://github.com/hardwario/lora-modem-abz/wiki/Type-ABZ-Modules) wireless module by Murata. The firmware provides an AT command interface backward compatible with Murata Modem, Murata's proprietary LoRaWAN firmware. The firmware can be used on all Type ABZ variants with an open (user-reprogrammable) microcontroller. Binary firmware images are provided for HARDWARIO [LoRa Module](https://shop.hardwario.com/lora-module/), Arduino [MKR WAN 1300](https://store-usa.arduino.cc/products/arduino-mkr-wan-1300-lora-connectivity), and [MKR WAN 1310](https://store.arduino.cc/products/arduino-mkr-wan-1310) boards.

## Main Features
## Main Features

* Support for [LoRaWAN 1.0.4](https://resources.lora-alliance.org/technical-specifications/ts001-1-0-4-lorawan-l2-1-0-4-specification), [LoRaWAN 1.1](https://resources.lora-alliance.org/technical-specifications/lorawan-specification-v1-1), and regional parameters [RP2-1.0.3](https://resources.lora-alliance.org/technical-specifications/rp2-1-0-3-lorawan-regional-parameters)
* Based on the most recent version (unreleased 4.7.0) of [LoRaMac-node](https://github.com/Lora-net/LoRaMac-node)
Expand Down
89 changes: 86 additions & 3 deletions src/cmd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,21 @@ static void set_appskey(atci_param_t *param)

static void get_appkey(void)
{
atci_print("+OK=");
atci_print_buffer_as_hex(find_key(APP_KEY), SE_KEY_SIZE);
EOL();
if ( sysconf.appkey_readable ) {
atci_print("+OK=");
atci_print_buffer_as_hex(find_key(APP_KEY), SE_KEY_SIZE);
EOL();
} else {
abort(ERR_UNSUPPORTED);
}
}

static void protect_appkey(atci_param_t *param)
{
sysconf.appkey_readable = 0;
sysconf_modified = true;
OK_();
}

static void set_appkey_10(atci_param_t *param)
{
Expand Down Expand Up @@ -2055,6 +2065,74 @@ static void get_session(void)
}



// Read and Store a User data into the NVM area
// AT$NVM=0 read data at address 0
// AT$NVM=0,223 write 223 at address 0
static void set_nvm(atci_param_t *param) {
janakj marked this conversation as resolved.
Show resolved Hide resolved
uint32_t adr, value;
if (!atci_param_get_uint(param, &adr)) abort(ERR_PARAM);
if (adr >= USER_NVM_MAX_SIZE) abort(ERR_PARAM);
if (param->offset < param->length) {
if (!atci_param_is_comma(param)) abort(ERR_PARAM);
if (!atci_param_get_uint(param, &value)) abort(ERR_PARAM);
if (value >= 256) abort(ERR_PARAM);
user_nvm.values[adr] = value;
userNvm_process();
OK("%d,%d",(int)adr,(int)value);
janakj marked this conversation as resolved.
Show resolved Hide resolved
} else {
OK("%d",user_nvm.values[adr]);
}
}

#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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@disk91 disk91 Nov 7, 2022 •
edited by janakj
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unclear on what you mean ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


static const atci_command_t cmds[] = {
{"+UART", NULL, set_uart, get_uart, NULL, "Configure UART interface"},
{"+VER", NULL, NULL, get_version_comp, NULL, "Firmware version and build time"},
Expand Down Expand Up @@ -2125,6 +2203,11 @@ static const atci_command_t cmds[] = {
{"$SESSION", NULL, NULL, get_session, NULL, "Get network session information"},
{"$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"},
janakj marked this conversation as resolved.
Show resolved Hide resolved
janakj marked this conversation as resolved.
Show resolved Hide resolved
{"$APKACCESS", protect_appkey, NULL, NULL, NULL, "Protect AppKey against read access"},
Copy link
Collaborator

@janakj janakj Nov 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this should also applied to AppSkey and NetSkey

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@disk91 disk91 Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#if MKR1310 == 1
{"$DISUART", disable_uart, NULL, NULL, NULL, "Disable UART"},
#endif
ATCI_COMMAND_CLAC,
ATCI_COMMAND_HELP};

Expand Down
4 changes: 4 additions & 0 deletions src/cmd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,8 @@ void cmd_ans(unsigned int margin, unsigned int gwcnt);
#define cmd_print atci_print
#define cmd_printf atci_printf

#if MKR1310 == 1
void process_uart_wakeup(void);
#endif

#endif // _CMD_H
12 changes: 12 additions & 0 deletions src/lpuart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ void lpuart_init(unsigned int baudrate)
}



static void init_gpio(void)
{
GPIO_InitTypeDef gpio = {
Expand Down Expand Up @@ -168,6 +169,17 @@ static void deinit_gpio(void)
}


void lpuart_disable(void) {
lpuart_flush();
HAL_UART_DMAPause(&port);
deinit_gpio();
}

void lpuart_enable() {
init_gpio();
HAL_UART_DMAResume(&port);
}

void HAL_UART_MspInit(UART_HandleTypeDef *port)
{
/* Enable peripherals and GPIO Clocks */
Expand Down
14 changes: 14 additions & 0 deletions src/lpuart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,18 @@ void lpuart_before_stop(void);
void lpuart_after_stop(void);


/*! @brief Disable LPUART1 Gpio for MKRWAN1310
*
* This function is specific to MKRWAN1310, it disable the lpuart pins to allow sharing the
* wires on the board with the SPI to use the onboarded flash.
*/
void lpuart_disable(void);

/*! @brief Reinit LPUART1 Gpio for MKRWAN1310
*
* This function is specific to MKRWAN1310, it reinit the lpuart pins to allow sharing the
* wires on the board with the SPI to use the onboarded flash.
*/
void lpuart_enable(void);

#endif /* __LPUART_H__ */
4 changes: 3 additions & 1 deletion src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ int main(void)
cmd_event(CMD_EVENT_MODULE, CMD_MODULE_BOOT);

while (1) {
#if MKR1310 == 1
process_uart_wakeup();
#endif
cmd_process();
lrw_process();
sysconf_process();
Expand Down Expand Up @@ -91,7 +94,6 @@ int main(void)
}

enable_irq();

// Invoke lrw_process as the first thing after waking up to give the MAC
// a chance to timestamp incoming downlink as quickly as possible.
lrw_process();
Expand Down
40 changes: 37 additions & 3 deletions src/nvm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "nvm.h"
#include <assert.h>
#include <string.h>
#include <strings.h>
#include <stm/include/stm32l072xx.h>
#include <loramac-node/src/mac/LoRaMacTypes.h>
#include <loramac-node/src/mac/LoRaMac.h>
Expand All @@ -11,7 +12,7 @@
#include "part.h"
#include "utils.h"

#define NUMBER_OF_PARTS 8
#define NUMBER_OF_PARTS 9


/* The following partition sizes have been derived from the in-memory size of
Expand All @@ -30,6 +31,7 @@
#define REGION1_PART_SIZE 32
#define REGION2_PART_SIZE 1536
#define CLASSB_PART_SIZE 32
#define USER_NVM_PART_SIZE 12
janakj marked this conversation as resolved.
Show resolved Hide resolved


// Make sure each data structure fits into its fixed-size partition
Expand All @@ -41,6 +43,7 @@ static_assert(sizeof(SecureElementNvmData_t) <= SE_PART_SIZE, "SecureElement NVM
static_assert(sizeof(RegionNvmDataGroup1_t) <= REGION1_PART_SIZE, "RegionGroup1 NVM data too long");
static_assert(sizeof(RegionNvmDataGroup2_t) <= REGION2_PART_SIZE, "RegionGroup2 NVM data too long");
static_assert(sizeof(LoRaMacClassBNvmData_t) <= CLASSB_PART_SIZE, "ClassB NVM data too long");
static_assert(sizeof(userNvm_t) <= USER_NVM_PART_SIZE, "User NVM data too long");


// And also make sure that NVM data fits into the EEPROM twice. This is
Expand All @@ -53,7 +56,8 @@ static_assert(
SE_PART_SIZE +
REGION1_PART_SIZE +
REGION2_PART_SIZE +
CLASSB_PART_SIZE
CLASSB_PART_SIZE +
USER_NVM_PART_SIZE
<= (DATA_EEPROM_BANK2_END - DATA_EEPROM_BASE + 1 - PART_TABLE_SIZE(NUMBER_OF_PARTS)) / 2,
"NVM data does not fit into a single EEPROM bank");

Expand All @@ -71,6 +75,8 @@ static part_block_t nvm = {

struct nvm_parts nvm_parts;

userNvm_t user_nvm = { 0 };

sysconf_t sysconf = {
.uart_baudrate = DEFAULT_UART_BAUDRATE,
.uart_timeout = 1000,
Expand All @@ -79,7 +85,8 @@ sysconf_t sysconf = {
.sleep = 1,
.device_class = CLASS_A,
.unconfirmed_retransmissions = 1,
.confirmed_retransmissions = 8
.confirmed_retransmissions = 8,
.appkey_readable = 1
};

bool sysconf_modified;
Expand Down Expand Up @@ -147,6 +154,11 @@ void nvm_init(void)
nvm_parts.classb.dsc->size != CLASSB_PART_SIZE)
goto retry;

if ((part_find(&nvm_parts.user, &nvm, "user") &&
part_create(&nvm_parts.user, &nvm, "user", USER_NVM_PART_SIZE)) ||
nvm_parts.user.dsc->size != USER_NVM_PART_SIZE)
goto retry;

size_t size;
const uint8_t *p = part_mmap(&size, &nvm_parts.sysconf);
if (check_block_crc(p, sizeof(sysconf))) {
Expand All @@ -155,6 +167,17 @@ void nvm_init(void)
} else {
log_debug("Invalid system configuration checksum, using defaults");
}

p = part_mmap(&size, &nvm_parts.user);
if (check_block_crc(p, sizeof(user_nvm))) {
log_debug("Restoring user data from NVM");
memcpy(&user_nvm, p, sizeof(user_nvm));
} else {
log_debug("Invalid user data checksum, using defaults");
user_nvm.magic = USER_NVM_MAGIC;
bzero(user_nvm.values,USER_NVM_MAX_SIZE);
}

return;

retry:
Expand Down Expand Up @@ -192,3 +215,14 @@ void sysconf_process(void)
sysconf_modified = false;
}


void userNvm_process(void)
{

if (update_block_crc(&user_nvm, sizeof(user_nvm))) {
log_debug("Saving user data to NVM");
if (!part_write(&nvm_parts.user, 0, &user_nvm, sizeof(user_nvm)))
log_error("Error while writing user data to NVM");
}

}
16 changes: 16 additions & 0 deletions src/nvm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ typedef struct sysconf
*/
uint8_t confirmed_retransmissions;

/* This is allowing to read the appKey from the serial line of not
* once set to false (0) it will not be possible to retrieve the appKey
*/
uint8_t appkey_readable:1;

uint32_t crc32;
} sysconf_t;

Expand All @@ -66,18 +71,29 @@ struct nvm_parts {
part_t region1;
part_t region2;
part_t classb;
part_t user;
};

#define USER_NVM_MAX_SIZE 4 // maximum allows values inside the User Nvm area
#define USER_NVM_MAGIC 0xD15C9101
typedef struct userNvm_s {
janakj marked this conversation as resolved.
Show resolved Hide resolved
uint32_t magic;
uint8_t values[USER_NVM_MAX_SIZE];
uint32_t crc32;
} userNvm_t;


extern struct nvm_parts nvm_parts;
extern sysconf_t sysconf;
extern bool sysconf_modified;
extern uint16_t nvm_flags;
extern userNvm_t user_nvm;

void nvm_init(void);

int nvm_erase(void);

void sysconf_process(void);
void userNvm_process(void);

#endif // _NVM_H_