drivers/at86rf2xx; spi security module (AES) by fabian18 · Pull Request #14113 · RIOT-OS/RIOT

fabian18 · 2020-05-21T17:07:02Z

Contribution description

The at86rf2xx IEEE 802.15.4 transceivers are capable to perform AES in hardware and support block cipher modes ECB and CBC.

This PR adds a pseudomodule in the at86rf2xx driver. You will be able to perform hardware encryption and decryption of blocks of data using ECB and CBC block ciphers. Nothing is changed from the actual driver.

Only SPI transceivers are supported, for now. Although for MCU integrated transceivers atmega128rfa1 and atmega256rfr2 it is even simpler and could be added later.
The SPI security module supports fast SPI, which means that you will read output block i, if you write input block i + 1 to the SPI bus.

Testing procedure

I provided a test in tests/driver_at86rf2xx_aes/.
Test vectors are taken from: RFC3602

Issues/PRs references

None

benpicco

Looks good!
Neat to have this feature.
Just a few questions:

do we need to take the radio state into account?
if we want to encrypt payload, do we really have to write the plaintext onto the device, read it back, and then send it? I would have imagined it would have a mode where it would handle encryption / decryption transparently.

benpicco · 2020-06-01T21:52:47Z

drivers/at86rf2xx/aes_spi.c

+static inline
+void at86rf2xx_spi_get_bus(const at86rf2xx_t *dev)
+{
+    spi_acquire(dev->params.spi, dev->params.cs_pin, SPI_MODE_0, dev->params.spi_clk);
+}
+
+static inline
+void at86rf2xx_spi_release_bus(const at86rf2xx_t *dev)
+{
+    spi_release(dev->params.spi);
+}


You could move those to at86rf2xx_internal.h if you want.
But might also be done in a later patch. I was wondering why you didn't just use at86rf2xx_sram_read() and friends, but I guess the problem there is that they will all acquire and release the bus.

Only recently I have learned that this is a bad idea (and even more so after the recent power optimizations after release() on sam0), so the right courser of action would be to convert the driver to not acquire() and release() the bus, just like you did here.

But that would be another PR.

but I guess the problem there is that they will all acquire and release the bus.

Yes.
And further at86rf2xx_sram_read() writes dummy bytes to the transceiver, while
at86rf2xx_sram_write() does not care what it reads from the transceiver.
For fast SPI, you read the result from the last block operation, while you write the next block.

drivers/at86rf2xx/aes_spi.c

jia200x · 2020-06-02T09:28:54Z

do we need to take the radio state into account?

No. The AES component is independent of the transceiver. It's indeed possible to encrypt/decrypt while sending/receiving (assuming the device is not sleeping).

if we want to encrypt payload, do we really have to write the plaintext onto the device, read it back, and then send it? I would have imagined it would have a mode where it would handle encryption / decryption transparently.

Yes :(. It's required to write the plaintext packet and then read it from SRAM since it's a "generic" AES crypto accelerator (intended to be used with IEEE802.15.4, but other use cases are also possible)

jia200x · 2020-06-02T09:34:36Z

drivers/at86rf2xx/aes_spi.c

+}
+
+static inline
+uint8_t _aes_status(at86rf2xx_t *dev)


can't this be replaced with the at86rf2xx_sram_xxx functions (from 'at86rf2xx_internal.c`)?

See https://github.com/RIOT-OS/RIOT/pull/14113/files#r433510445

jia200x · 2020-06-02T09:35:36Z

drivers/at86rf2xx/aes_spi.c

+}
+
+static inline
+void _aes_open_read(at86rf2xx_t *dev, uint8_t addr)


jia200x · 2020-06-02T09:36:07Z

drivers/at86rf2xx/aes_spi.c

+}
+
+static inline
+void _aes_open_write(at86rf2xx_t *dev, uint8_t addr)


I cannot reuse them because, they acquire / release the bus in each call and at86rf2xx_sram_write() ignores what is read from the transceiver, which is important for fast SPI.

hmmm I would say that these are "presentation layer" functions (not specific to AES). Maybe they can be added as generic functions? What do you think?

Generally I agree, they could be moved to at86rf2xx_internal because they are not related to the security module. But I am not sure if I should do this because nothing else uses it, so far.

On the one side it touches the actual driver, and I think this should be done, if some day someone decides to modify the at86rf2xx SPI functions in a way that they do not acquire / release the SPI bus for every call. Right now I see no benefit of moving them.

On the other side it doesn´t harm because it is a small change.

I would keep them in place for now.

fabian18 · 2020-06-02T17:46:53Z

Git is showing a weird diff for 39c23ac.
But it just does the

    if (!nblocks) {
        return 0;
    }

thing.

drivers/at86rf2xx/include/at86rf2xx_aes.h

drivers/at86rf2xx/aes_spi.c

benpicco · 2020-08-10T22:01:12Z

tests/driver_at86rf2xx_aes/main.c

+        else {
+            puts("CBC test 3 failed");
+        }
+    }


Would be good if the test would print SUCCESS at the end.
Then you can also add a tests/01-run.py with

#!/usr/bin/env python3 import sys from testrunner import run def testfunc(child): child.expect('START') child.expect('ECB test 1 done') child.expect('ECB test 2 done') child.expect('CBC test 1 done') child.expect('CBC test 2 done') child.expect('CBC test 3 done') child.expect('SUCCESS') if __name__ == "__main__": sys.exit(run(testfunc))

so we can have make test work for automatic tests.

Hm looks like you'll have to add a puts("START"); at the beginning of the test too.
This and chmod a+x tests/01-run.py.

Then make test will work.

Or just remove matching start, test_utilis_interactive_sync will already match the start for you.

ups forgot that ...

tests/driver_at86rf2xx_aes/main.c

drivers/at86rf2xx/aes_spi.c

benpicco · 2020-08-10T22:16:04Z

drivers/at86rf2xx/aes_spi.c

+    }
+    uint8_t cfg = AT86RF2XX_AES_CTRL_AES_MODE__ECB |
+                  AT86RF2XX_AES_CTRL_AES_DIR__ENC;
+    uint8_t mirror = cfg | AT86RF2XX_AES_CTRL_MIRROR_MASK__AES_REQUEST;


What's that mirror about?

The mirror must be the same value as the cfg and is transfered to the slave to start an AES operation, if AT86RF2XX_AES_CTRL_MIRROR_MASK__AES_REQUEST is set.

benpicco · 2020-08-10T22:19:13Z

drivers/at86rf2xx/aes_spi.c

+    while (!(status & AT86RF2XX_AES_STATUS_MASK__AES_DONE)) {
+        AES_DEBUG("status: %02x\n", status);
+        status = _aes_status(dev);
+    }


Could we still get the AT86RF2XX_AES_STATUS_MASK__AES_ER bit set here?

No the error bit is set before the transceiver has processed a data block. There are two cases:

The delay between initiating an AES operation and sending the next cfg to AT86RF2XX_REG__AES_CTRL was too short. Meaning the transceiver did not have enough time to process the current block.

Less then 16 bytes of data have been sent to the transceiver. This never happens in the code, so this case cannot arrive.

Better add that as a comment so in the future this question doesn't come up again :)

Actually the first case should not arrive either because I am polling if the operation is complete.
I could make an assert() that the error bit is never set and change the return types of all functions to be void because that was the only thing that I expected that could go wrong. Every time I witnessed the error bit was set, there was a mistake in the code.
Or there may be something that I am not aware of and the current implementation would be a little safer.
But I don´t think so. Even when I reduce AT86RF2XX_AES_DELAY_US, the error is not set because the polling is safe.
So I would like to turn it into an assert()

drivers/at86rf2xx/include/at86rf2xx_aes.h

benpicco · 2020-08-10T22:33:38Z

This looks pretty good.
Some more comments would be nice as the code is pretty dense.

I'd like to have this, though it's not a requirement, it would sure encourage the work on MAC layer security. :)

fabian18 · 2020-08-12T11:44:43Z

Now I think it´s time to squash.

benpicco

Looks good to me and the automatic test is also working.
Please squash.

benpicco · 2020-08-12T15:26:04Z

tests/driver_at86rf2xx_aes/Makefile

+DISABLE_MODULE += auto_init_at86rf2xx
+
+# define the driver to be used for selected boards
+ifneq (,$(filter samr21-xpro,$(BOARD)))
+  DRIVER := at86rf233
+endif
+ifneq (,$(filter iotlab-m3 fox,$(BOARD)))
+  DRIVER := at86rf231
+endif
+ifneq (,$(filter mulle,$(BOARD)))
+  DRIVER := at86rf212b
+endif
+
+# use the at86rf231 as fallback device
+DRIVER ?= at86rf231
+
+# include the selected driver
+USEMODULE += $(DRIVER)


Do we actually need the base driver here?

for this in main.c:

at86rf2xx_setup(&dev, &at86rf2xx_params[0]); dev.netdev.netdev.event_callback = _event_cb; if (dev.netdev.netdev.driver->init(&dev.netdev.netdev) != 0) { return EXIT_FAILURE; }

Yea but do we need it?
I mean if we have to init the radio to use crypto that's fine.
But if we just init it and then never use it, we might as well drop it.

I don´t think we need the driver capability but it initializes the params, SPI and checks if SPI is working.
You mean I can copy the params and init SPI manually? Then the driver is not needed. Is this better?

Then I would have to make an own driver rather then a pseudomodule, right?

A never mind then.
A separate init function would be possible, but that's not worth the hassle now.

If someone really wants to use the crypto without the radio, that could be added later.

fabian18 · 2020-08-12T15:46:57Z

I am going to insert two empty lines in the python script to make travis happy and squash right away.

fabian18 · 2020-08-12T16:51:29Z

Then I am going to add a Makefile.ci, to make murdock happy.

fabian18 · 2020-08-13T06:34:51Z

Hooray 🎉 , thanks for merging.

fabian18 requested review from MichelRottleuthner, aabadie, daniel-k, fjmolinas, jia200x, leandrolanzieri, miri64 and smlng as code owners May 21, 2020 17:07

benpicco added Area: security Area: Security-related libraries and subsystems Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 21, 2020

benpicco reviewed Jun 1, 2020

View reviewed changes

drivers/at86rf2xx/aes_spi.c Outdated Show resolved Hide resolved

jia200x reviewed Jun 2, 2020

View reviewed changes

benpicco reviewed Jul 27, 2020

View reviewed changes

drivers/at86rf2xx/include/at86rf2xx_aes.h Outdated Show resolved Hide resolved

benpicco reviewed Jul 27, 2020

View reviewed changes

drivers/at86rf2xx/aes_spi.c Outdated Show resolved Hide resolved

benpicco reviewed Aug 10, 2020

View reviewed changes

tests/driver_at86rf2xx_aes/main.c Outdated Show resolved Hide resolved

benpicco reviewed Aug 10, 2020

View reviewed changes

drivers/at86rf2xx/aes_spi.c Show resolved Hide resolved

benpicco reviewed Aug 10, 2020

View reviewed changes

drivers/at86rf2xx/include/at86rf2xx_aes.h Show resolved Hide resolved

benpicco reviewed Aug 10, 2020

View reviewed changes

drivers/at86rf2xx/include/at86rf2xx_aes.h Outdated Show resolved Hide resolved

benpicco approved these changes Aug 12, 2020

View reviewed changes

drivers/at86rf2xx: SPI AES security module

1fca10c

fabian18 force-pushed the at86rf2xx_spi_security_module branch from 50ae1b1 to 6bb3d81 Compare August 12, 2020 15:20

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 12, 2020

benpicco reviewed Aug 12, 2020

View reviewed changes

fabian18 force-pushed the at86rf2xx_spi_security_module branch from 6bb3d81 to 7fe64c5 Compare August 12, 2020 15:48

tests/driver_at86rf2xx_aes: Test for at86rf2xx AES security module

74eec82

fabian18 force-pushed the at86rf2xx_spi_security_module branch from 7fe64c5 to 74eec82 Compare August 12, 2020 16:55

benpicco merged commit add10b5 into RIOT-OS:master Aug 12, 2020

benpicco mentioned this pull request Aug 17, 2020

netdev: add netdev_register() to keep track of netdev IDs #13746

Merged

chudov mentioned this pull request Feb 17, 2021

drivers/at86rf2xx_aes; MCU integrated transciever support #16040

Closed

fabian18 deleted the at86rf2xx_spi_security_module branch January 11, 2025 15:35

Conversation

fabian18 commented May 21, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jia200x commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian18 commented Jun 2, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benpicco commented Aug 10, 2020

Uh oh!

fabian18 commented Aug 12, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian18 Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian18 Aug 12, 2020 •

edited

Loading