pkg/semtech-loramac: rework how session keys are get/set

[aabadie]

Contribution description

This PR fixes #11626.

Although the reason is still unclear to me, I noticed that using Loramac MIB get/set functions to get/set network/application session keys were not working as expected after reading the values from EEPROM.

The idea of this PR is to skip the use of Loramac MIB get/set functions and patch the package to get a global access to the underlying arrays.
Even though this is not very nice, it avoids having to memcpy twice a 16bytes array for each keys.

I tested this change in the im880b board (stm32l1 + sx1272) and b-l072z-lrwan1 and on both it's working like a charm.

Note that I'm not sure this is the best fix, but at least it works.

Testing procedure

• Run tests/pkg_semtech-loramac on b-l072z-lrwan1 and lobaro-lorabox boards
• For OTAA, test this workflow:
  • Ensure EEPROM is erased (loramac erase) and reboot
  • set deveui, appeui, appkey
  • join the network: loramac join otaa
  • save the parameters to the EEPROM loramac save and reboot
  • send something: it should work
• For ABP, test this workflow:
  • Ensure EEPROM is erased (loramac erase) and reboot
  • set devaddr, appskey, nwskey
  • join the network: loramac join abp
  • save the parameters to the EEPROM loramac save and reboot
  • eventually configure the rx2_dr (for TTN set it to 3) and send something: it should work

Issues/PRs references

fixes #11626

[jia200x]

hi @aabadie

do you have the steps to reproduce the original issue in b-l072z-lrwan1?

I'm not sure if it's a good idea to just access internal values of the packet. It makes it harder to maintain and I think the functions might not be the real cause of not restoring the keys properly. In fact, the LoRaMacMibSetRequestConfirm function does memcpy (see https://github.com/Lora-net/LoRaMac-node/blob/1cdd9ccec4c9f05b616e7112059be4a9e358c571/src/mac/LoRaMac.c#L2855).

I would check the return status of the LoRaMacMibSetRequestConfirm in order to check what's going on there

[aabadie]

do you have the steps to reproduce the original issue in b-l072z-lrwan1?

You can follow the testing procedure described here. On master, after the last reboot step (before send), you get a hardfault on the lobaro-lorabox (and im880b). With this PR, it doesn't happen, everything works as expected.
Note also that the issue doesn't happen when running on b-l072z-lrwan1.

the functions might not be the real cause of not restoring the keys properly

I think the same but when one calls them to set the AppSKey and NwkSKey just after reading the keys from EEPROM, they don't work as expected: the value read from EEPROM is correct but after, the value copied to the corresponding static variable is lost for some reason.
This doesn't happen when calling the set function from the shell function.
It could be some context issue but both semtech_loramac_set_appskey/semtech_loramac_set_nwkskey functions are called from the main thread (in loramac_init and in the shell function).
Is the mutex not doing his job correctly ? The fact that 2 calls to memcpy are done to update the static variable seems overkill to me (it the same with other parameters actually).

I would check the return status of the LoRaMacMibSetRequestConfirm in order to check what's going on there

I did a lot of debugging in the function itself to verify what arrays were copied to the static variables containing the key. Normal code path was used so checking the return value won't help.

I won't be able to work again on this before the release unfortunately.

[fjmolinas]

Looking into the code it seems to me the problem is actually here:

    MibRequestConfirm_t mibReq;
    mibReq.Type = MIB_APP_SKEY;
    memcpy(mibReq.Param.AppSKey, skey, LORAMAC_APPSKEY_LEN);
    LoRaMacMibSetRequestConfirm(&mibReq);

mibReq.Param.AppSKey is an uninitialized pointer, so calling memcpy here is incorrect. What should actually be done is simply:

mibReq.Param.AppSKey = (uint8_t *) skey;

[jia200x]

#11847 solves the issue in a less intrusive way. Shall we close this one?

[aabadie]

#11847 was merged, so let's close this.