üApp: picar-s: migrate HMAC functionality from `i2c-dev` into `i2c-bcm2708`

This task will move the HMAC computation from i2c-dev driver into i2c-bcm2708 low-level I2C driver. The following are the proposed sub-tasks (we will expand on the bullets as we proceed):

  1. We will be working with forks and respective branches of the following repositories:

    • uberspark/uberxmhf.git on branch develop
    • uberspark/uberspark.git on branch develop
    • uberspark/uobjcoll-raspberrypi-linux-i2c-bcm2835.git on branch uobjcoll-4.4.y

    Ensure the aforementioned forks/branches are upto date with the upstream repositories. The following gives an example of how to sync your fork of uberspark/uberxmhf.git; the same can be applied to other repositories. Note these commands have to be applied on a local checkout of your fork.

    git remote add upstream https://github.com/uberspark/uberxmhf.git
    git fetch -a upstream
    git checkout develop 
    git pull upstream develop 
    git push origin develop
    
  2. Build and install micro-hypervisor and I2C drivers (i2c-dev and i2c-bcm2708) from the above forks/branches and ensure baseline line-following functionality works.

  3. Migrate the HMAC logic from i2c-dev function i2cdev_read into i2c-bcm2708 function bcm2708_i2c_master_xfer.

Once the migration is complete, the line-following functionality should work as before.

PR(s):

Merge(s):

I assume that we want to have the khcall() call in order to calculate the HMAC in i2c-bcm2708.c as opposed to calculating it in the driver.

Yes that is correct. But we still need to retain some of the buffer size increase logic (to account for HMAC appending) within i2c-dev since it might be difficult to resize the message buffer in the low-level i2c-bcm2708 driver.

Yes, thanks for reminding me. It is always tricky who does what with memory. That is why I always look at dmesg to see if I made the kernel unhappy or not …

1 Like

I have the code calculating HMAC at the lowest level i2c driver.
The testing shows it worked well. At least it is not worse than when the logic was at the high level driver.
Ready to submit a PR.
This looks like a solid argument for security when wrapping it with HMAC at the origin of data and giving no advantage to the intruder (Alice).

Here is the PR:

Amazing stuff! Thanks @antonhristozov!

A few questions/clarifications below:

  1. Why do we still have khcall within i2c-dev (lines 218–230)? There is already a khcall within i2c-bcm2708 which computes the HMAC, so perhaps this needs to be removed?

  2. Not sure what the purpose of lines 193–200 is within i2c-dev, especially if we remove the khcall above?

All the code that is in the #ifdef UOBJCOLL blocks is not compiled now.
We can remove it, but I thought that it could stay there in case it is needed,
Let me know if you prefer that I remove these unused blocks.
I am not biased and can support either way.
Flexibility is why we like working on software, right!

No worries. I think you can leave it in there for now :); I did not notice the Makefile which removes the UOBJCOLL definition.

However, I believe we should migrate the hmac_sha256_memory call currently in i2c-dev to i2c-bcm2708. This way, the HMAC is computed at the low-level driver.

But the HMAC call in i2c-dev.c is no longer used.
The lowest level driver i2c-bcm2708.c is making a hyper call for calculating the HMAC.
Maybe I should clear i2c-dev.c to avoid any confusion in reading the code.

I think you are still passing HMAC_DIGEST build option to i2c-dev which I believe enables lines 231–235 that invokes hmac_sha256_memory, which of course is redundant in our case.

Not true, because we do not get into the if, since the returned bytes are not equal to msg_size. I am cleaning up the code to avoid confusion and will retest.

1 Like

I think it might be useful to enable building of i2c-bcm2708 with UOBJCOLL and HMAC_DIGEST options just like i2c-dev. That way, if we want to test just HMAC performance without hypercall, we can do so.

Cleaned up i2c-dev.c as part of the PR. Retested and it all works well.

Thanks!. Can you also add support for the above? i.e., we should be able to build i2c-bcm2708 without IOUOBJ and just HMAC_DIGEST and it should compute the HMAC via hmac_sha256_memory without the hypercall (just like we had in i2c-dev).

Actually IOUOBJ controls our IO only and is orthogonal to HMAC.
We can add UOBJCOL like we had in i2c-dev.c and do a khcall when defined and otherwise local HMAC calculation.

That will be great. Thanks @antonhristozov!

Added support for HMAC in the low level driver using local calculation and one through khcall().
Tested both scenarios, which are commanded by a compilation flag in the Makefile.
This should be good to go.

PR and merge info linked to OP; PR merged upstream. Closing topic thread. Thanks @antonhristozov!