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.

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

Add support for SLB9760 Iridium / LetsTrust TPM boards #2585

Closed
wants to merge 3 commits into
base: rpi-4.18.y
from

Conversation

Projects
None yet
5 participants
@PeterHuewe
Contributor

PeterHuewe commented Jun 14, 2018

This patchset adds support for the SLB9670 based TPM2.0 eval boards which can be used as a secure key storage and hwrng.
These boards are available as "Iridium SLB9670" by Infineon and "LetsTrust TPM" pi3g.
Working upstream driver is available since about kernel v4.10 - so enabling support on older kernels is also possible.

Signed-off-by: Peter Huewe peterhuewe@gmx.de

@PaulKissinger

This comment has been minimized.

PaulKissinger commented Jun 15, 2018

Tested and verified on a RaspberryPi 3b and 3b+ with LetsTrust-TPM.

Tested-by: Paul Kissinger PaulKissinger@letstrust.de

@PaulKissinger

This comment has been minimized.

PaulKissinger commented Jun 16, 2018

Now tested and verified on a combined image for RaspberryPi Zero W, Raspberry Pi 2b, RaspberryPi 3b and Raspberry Pi 3b+.
All tests have been done with LetsTrust-TPM and the Iridium-Board.

Tested-by: Paul Kissinger PaulKissinger@letstrust.de

@pelwell

This comment has been minimized.

Contributor

pelwell commented Jun 16, 2018

Although I don't have a TPM to test I did build a kernel with the updated configuration. Even though the new settings are all =m they do cause a small increase in static kernel size and larger decrease in free memory (7MB?), even when the modules aren't loaded. Without this concern I would have merged already.

@PeterHuewe @PaulKissinger Have you looked at the impact on the memory footprint for non-users of the TPM? Does my figure of 7MB sound right to you? How do you feel about such an increase, @popcornmix?

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jun 16, 2018

Thanks for your response - to be honest I did not have a look at the decrease in free memory, as I did not really expect it, since the TPM drivers are not that big, so this was quite suprising.
The reason behind this is probably the automatic selection of SECURITYFS, which might be beneficial to other use cases as well.

So by enabling TCG_TIS_SPI the resulting config changes with regard to:

 SECURITYFS n -> y
 TCG_TPM n -> m
+CRYPTO_HASH_INFO y
+HW_RANDOM_TPM m
+TCG_TIS_CORE m
+TCG_TIS_SPI m

I will try to find out what increases the RAM usage.

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from 76cf0fe to 464be8a Jun 18, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jun 18, 2018

@pelwell I tried to understand what's going on, but I could not reproduce the issue.
The memory usage (free -k) between with the patchset and without are in my case nowhere near 7mb in difference. I just see 732 kb in used memory difference - which to some extend might be even caused by something else.
https://paste.debian.net/1029806/
https://paste.debian.net/1029807/

Also the kernel virtual memory map looks identical - see dmesg
https://paste.debian.net/1029804/ (without support) vs
https://paste.debian.net/1029805/ (with support)

I also did a code review on securityfs and the tpm code, and nothing justified a 7MB increase (no big mallocs or huge arrays). (which would have come to a suprise to me as the (former) tpm subsys maintainer)

Can you help me reproduce the issue / retest on your side?
That would be really great!
Otherwise you can probably just rebase and merge.
Thanks!

@PaulKissinger

This comment has been minimized.

PaulKissinger commented Jun 18, 2018

Hi @pelwell,
if your internet connection faster than your CPU, so you could use the uploaded images on my site:
http://www.letstrust.de/uploads/2018-04-18-raspbian-stretch_rpi-4.17.y_with_tpm_support.zip
http://www.letstrust.de/uploads/2018-04-18-raspbian-stretch_rpi-4.17.y_without_tpm_support.zip

I see the same what @PeterHuewe describes.

I hope the images are a little usefull for you.

Thanks in advance!

@PeterHuewe PeterHuewe force-pushed the PeterHuewe:tpm_tis_spi_rpi branch from 95d6144 to 6805e33 Jun 19, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jun 19, 2018

@pelwell - rebased and tested on latest rpi-4.17.y to make it easier for merging.
Shall I post them for other branches as well? (4.14 ?)

@pelwell

This comment has been minimized.

Contributor

pelwell commented Jun 20, 2018

Building my own kernels with and without those patches I get:

Without:
              total        used        free      shared  buff/cache   available
Mem:         948316       86864      731344       13624      130108      796940
Swap:             0           0           0

With:
              total        used        free      shared  buff/cache   available
Mem:         948316       87040      730196       13624      131080      796628
Swap:             0           0           0

which shows 1.1MB more memory used (but only 320KB less "available"). That may not sound much on a Pi 3 but it is on a Model B with only 256MB to start with.

This currently sounds like a very niche application

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jun 20, 2018

320kb sounds much nicer than the original 7MB :) - and 320kb does not sound too bad.
We really have to look at either the used or the available columns which indicate a 176k / 312k difference.
I'm not sure how stable these numbers are though and whether they are also affected by other factors like things happening at boot up, network traffic etc...

The 256mb limit only affects "Pi 1 Model B" before produced before October 2012 - I'm not sure how many of them will upgrade to recent kernels.

If you really think this is a big blocking point - what do you think if we atleast enable TPM support for 2B/3B/3B+ (bcm2709_defconfig + bcmrpi3_defconfig) and leave it deactivated for (Zero/Zero WH/ Pi 1) (bcmrpi_defconfig) ?
Of course it would be sad to see the support not being enabled on the Zeros though...

Thanks,
Peter

@pelwell

This comment has been minimized.

Contributor

pelwell commented Jun 20, 2018

I'm going to say no. Feel free to come back with a more persuasive argument and more supporters, or to appeal to a higher power - I'm prepared to be outvoted or overruled - but the cost to non-users (who would massively outnumber the users) feels too high.

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from 369d435 to 6eb5214 Jun 27, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jun 28, 2018

Hi @pelwell - thanks for your honest feedback. I share your concerns about ram usage, so I sat down the last few days to debug the issue. The topic really caught my attention.

All following tests were done on 0ca2dfcfb836b5bc16fdd9d917c0da2f8cd7063d overlays: Add gpio-no-irq overlay with bcm2709_defconfig. I used the arm-bcm2708-linux-gnueabi toolchain from https://github.com/raspberrypi/tools
I copied the unmodified version to v4.17-original and the version with TPM support to v4.17-tpm-rand-spi for the following analysis.

I was following the guidelines from the former Linux Tiny Project: https://elinux.org/Kernel_Size_Tuning_Guide

The two kernels differ in the following CONFIG options:
$ ./scripts/diffconfig v4.17-original/.config v4.17-tpm-rand-spi/.config

SECURITYFS n -> y
TCG_TPM n -> m
+HW_RANDOM_TPM y
+TCG_ATMEL n
+TCG_TIS n
+TCG_TIS_CORE m
+TCG_TIS_I2C_ATMEL n
+TCG_TIS_I2C_INFINEON n
+TCG_TIS_I2C_NUVOTON n
+TCG_TIS_SPI m
+TCG_TIS_ST33ZP24_I2C n
+TCG_TIS_ST33ZP24_SPI n
+TCG_VTPM_PROXY n
+TRUSTED_KEYS n

As the modules are not loaded by non-users, we can concentrate on the vmlinux / zImage and its built-ins only.

Running $ ./scripts/bloat-o-meter -c v4.17-original/vmlinux v4.17-tpm-rand-spi/vmlinux
I get the following output

add/remove: 10/0 grow/shrink: 0/0 up/down: 1148/0 (1148)
Function                                     old     new   delta
securityfs_create_dentry                       -     456    +456
securityfs_remove                              -     168    +168
securityfs_create_symlink                      -     152    +152
securityfs_init                                -      92     +92
securityfs_evict_inode                         -      68     +68
fill_super                                     -      64     +64
securityfs_create_file                         -      52     +52
securityfs_create_dir                          -      52     +52
get_sb                                         -      40     +40
__initcall_securityfs_init1                    -       4      +4
Total: Before=7411396, After=7412544, chg +0.02%
add/remove: 3/0 grow/shrink: 0/0 up/down: 36/0 (36)
Data                                         old     new   delta
fs_type                                        -      28     +28
mount_count                                    -       4      +4
mount                                          -       4      +4
Total: Before=1464788, After=1464824, chg +0.00%
add/remove: 9/0 grow/shrink: 1/0 up/down: 233/0 (233)
RO Data                                      old     new   delta
securityfs_super_operations                    -     100    +100
__kstrtab_securityfs_create_symlink            -      26     +26
__kstrtab_securityfs_create_file               -      23     +23
__kstrtab_securityfs_create_dir                -      22     +22
__kstrtab_securityfs_remove                    -      18     +18
files                                        108     120     +12
__ksymtab_securityfs_remove                    -       8      +8
__ksymtab_securityfs_create_symlink            -       8      +8
__ksymtab_securityfs_create_file               -       8      +8
__ksymtab_securityfs_create_dir                -       8      +8
Total: Before=597365, After=597598, chg +0.04%

which is basically a nm --sort-sizes and grepping the results for the different categories and diffing them.

The symbols above all come from security/inode.c.

Since some parts of it might be stripped out again after linking we have to look at the diff of size -A of the vmlinux files (I left out the equal ones here for brevity):

	v4.17-original	v4.17-tpm-rand-spi	
                v4.17-original v4.17-tpm-rand-spi
          section         size             size    diff
            .text      7048200          7049256    1056
    __ksymtab_gpl        30600            30632      32
    __kcrctab_gpl        15300            15316      16
__ksymtab_strings       152371           152463      92
         __modver         1488             1348    -140
  .ARM.unwind_idx       244232           244304      72
  .ARM.unwind_tab       365672           365780     108
       .init.text       317476           317568      92
            .data       614820           614884      64
            Total     11921031         11922423    1392

Since I don't know what the __modver really is for, I will re-add these 140 bytes, since init sections are discarded after boot we can subtract this: 1392+140-92 = 1440 bytes static memory increase.

Now it comes to dynamic memory usage - which is of course much harder - so I had a look at the involved kernel code.

The assumption is still that non-users do not load the modules, and since they don't care also do not mount securityfs.
Thus only functions that are automatically called during boot-up are considered, i.e. init functions - which we have exactly 1: securityfs_init(void) (see https://elixir.bootlin.com/linux/latest/source/security/inode.c#L325 for details)

This function does exactly two things:

  1. create an sysfs mount point, which is an empty directory, which allocates an struct kernfs_node and adds this to a list.
    Although I don't know the exact size of the struct kernfs_node I doubt that it is even in the range of a few hundred bytes.
    The struct is defined here https://elixir.bootlin.com/linux/latest/source/include/linux/kernfs.h#L128

  2. register the filesystem, this merely adds the pointer to the kernel list of file systems, no allocation.

So adding static + dynamic usage does get us in the range of maybe 2k.

This also matches with our test-series we conducted over the last few days.
We booted the pi and logged the free -k output fresh after boot, after 10 seconds, 30 seconds, 60 seconds, then rebooted.
Comparing the values (e.g. every fresh after boot data point) the values for free, used, available vary a lot - on the exact same kernel - and even more so across the two different kernels.
Sometimes we even get lower values on the one with TPM support - free -k is not reliable enough for this, unfortunately.
If you think it helps I can run a new boot series over the weekend and generate statistics on them.

@pelwell @popcornmix what do you think? Is this analysis thorough enough and the added memory acceptable?

If you are still concerned, I can also look into removing the securityfs dependency, thus resulting in zero changes to the vmlinux binary (i.e. only new modules).

@PeterHuewe PeterHuewe force-pushed the PeterHuewe:tpm_tis_spi_rpi branch from 396f2af to 6d46c46 Jun 28, 2018

@ghollingworth

This comment has been minimized.

Contributor

ghollingworth commented Jun 29, 2018

Why isn't securityfs implemented as a module?

This would solve the problem...

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from 9f54ddc to 9467e9f Jul 9, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jul 10, 2018

@ghollingworth - SECURITYFS is the base filesystem for all the LSMs like apparmor and tomoyo.
I doubt that converting it to a module is possible without big efforts.
However I tried it out and I can drop savely the select SECURITYFS from the TPM kconfig as it is not strictly necessary - this would solve the problem as well.
I will create a patch for this and update the PR - now only TPM modules are built.

@PeterHuewe PeterHuewe force-pushed the PeterHuewe:tpm_tis_spi_rpi branch from 6d46c46 to 8cd44e4 Jul 10, 2018

@ghollingworth

This comment has been minimized.

Contributor

ghollingworth commented Jul 11, 2018

You've changed KConfig which is an upstream file, you should submit this change upstream first and if it is accepted then we'll take it. We're continuously trying to reduce our differences to the upstream kernel.

Have you discussed the change to a module with the maintainer of the security? I'm guessing the correct person to contact is Chris Wright

https://github.com/torvalds/linux/blob/master/MAINTAINERS#L8324

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Jul 11, 2018

Of course it would be easier to accept the max. 2k ram increase of securityfs than to change upstream - but nevertheless I will try it.

Both, changing the TPM Kconfig and converting securityfs to a module are upstream changes - so it will take a while to adresss either of them / have them upstream.
Since the select SECURITYFS change is much simpler and I have a good standing in the TPM subsystem I will try to adress this first. Jarkko is currently on anual leave, so it will take a few weeks.

@zveriu

This comment has been minimized.

zveriu commented Jul 14, 2018

Greatly looking forward for the "support for SLB9760 Iridium / LetsTrust TPM boards" in the official RPi branch/images, thanks in advance to all involved.

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch 2 times, most recently from 9a564ac to 26ea22f Jul 20, 2018

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from 26ea22f to 1522364 Jul 30, 2018

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from 1522364 to a1cb21b Aug 7, 2018

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch from a1cb21b to 7993b10 Aug 14, 2018

@popcornmix popcornmix force-pushed the raspberrypi:rpi-4.17.y branch 2 times, most recently from 3ef8aa7 to b0b5a34 Aug 22, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Oct 8, 2018

Hi,
here's the update:
As discussed I created a patch to remove the hard dependency on SECURITYFS.
The patch was accepted by the TPM Maintainer (Jarkko) and was pulled by the Security Maintainer (James Morris) a few days ago.
The patch is now included in linux-next and will most probably go in during the next merge window.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/char/tpm?h=next-20181008
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/char/tpm?h=next-20181008&id=2f7d8dbb11287cbe9da6380ca14ed5d38c9ed91f
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/commit/?h=next-tpm&id=2f7d8dbb11287cbe9da6380ca14ed5d38c9ed91f

@pelwell @ghollingworth : how shall we proceed here? Do we have to wait for the next merge window / next kernel version with the patch, or can I create an updated pull request (including the patch) now? Which kernel versions should we target for backporting?

I would propose the same approach as with stable commits, thus including the upstream commit id in the backport.

@pelwell

This comment has been minimized.

Contributor

pelwell commented Oct 8, 2018

Congratulations on getting your patch accepted - it always feels like such a mission.

I'm happy (in principle) to take a Pull Request now. We've already moved on to rpi-4.18.y, with rpi-4.19.y gradually coming online; I'd target 4.18 for now and we'll cherry-pick as necessary. Please do include the "commit upstream." line at the start of the commit message.

PeterHuewe added some commits Sep 3, 2018

tpm: Make SECURITYFS a weak dependency
commit 2f7d8db upstream.

While having SECURITYFS enabled for the tpm subsystem is beneficial in
most cases, it is not strictly necessary to have it enabled at all.
Especially on platforms without any boot firmware integration of the TPM
(e.g. raspberry pi) it does not add any value for the tpm subsystem,
as there is no eventlog present.

By turning it from 'select' to 'imply' it still gets selected per
default, but enables users who want to save some kb of ram by turning
SECURITYFS off.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Enable TPM TIS SPI support for TPM1.2 and TPM2.0 chips
This patch enables the support for SPI TPMs which follow the TCG TIS
FIFO/PTP specification like the SLB9670.
In order to decrease ram usage the weak dependency on CONFIG_SECURITFS
is explictly set to 'n'.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
Add overlay for SLB9760 Iridium /LetsTrust TPM
Device Tree overlay for the Infineon SLB9670 Trusted Platform Module add-on
boards, which can be used as a secure key storage and hwrng.
available as "Iridium SLB9670" by Infineon and "LetsTrust TPM" by
pi3g.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>

@PeterHuewe PeterHuewe force-pushed the PeterHuewe:tpm_tis_spi_rpi branch from 8cd44e4 to cf5cbb8 Oct 11, 2018

@PeterHuewe PeterHuewe changed the base branch from rpi-4.17.y to rpi-4.18.y Oct 11, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Oct 11, 2018

@pelwell Ready to merge :)

 $ ./scripts/diffconfig config-rpi-4.18-original config-rpi-4.18-tpm_tis_spi_rpi 
 TCG_TPM n -> m
+HW_RANDOM_TPM y
+TCG_ATMEL n
+TCG_TIS n
+TCG_TIS_CORE m
+TCG_TIS_I2C_ATMEL n
+TCG_TIS_I2C_INFINEON n
+TCG_TIS_I2C_NUVOTON n
+TCG_TIS_SPI m
+TCG_TIS_ST33ZP24_I2C n
+TCG_TIS_ST33ZP24_SPI n
+TCG_VTPM_PROXY n
+TRUSTED_KEYS n


$ ./scripts/bloat-o-meter vmlinux-rpi-4.18-original vmlinux-rpi-4.18-tpm_tis_spi_rpi
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Function                                     old     new   delta
Total: Before=9119118, After=9119118, chg +0.00%

@pelwell

This comment has been minimized.

Contributor

pelwell commented Oct 12, 2018

Merged offline. Thanks for persevering with this and finding a mutually acceptable solution.

@pelwell pelwell closed this Oct 12, 2018

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Nov 5, 2018

Thanks @pelwell for merging this and fixing up the dts.
The patch also made it into v4.20-rc1 in the meantime.

@PeterHuewe

This comment has been minimized.

Contributor

PeterHuewe commented Dec 3, 2018

Hi @pelwell @ghollingworth - will this be automatically added for 4.19 / 4.20 or shall I post a new pull request for these kernel versions?
A backport to 4.14 probably does not make sense anymore?

@pelwell

This comment has been minimized.

Contributor

pelwell commented Dec 4, 2018

Thanks for the prompt. We've been burning through kernel versions so quickly that the 4.18 to 4.19 change hasn't had quite enough attention.

TPM support should now be in 4.19, 4.20 and 4.14 for good measure.

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Dec 4, 2018

kernel: Bump to 4.19.6
kernel: V4L2 codec interface
See: raspberrypi/linux#2755

kernel: Add support for SLB9760 Iridium / LetsTrust TPM boards
See: raspberrypi/linux#2585

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Dec 4, 2018

kernel: Bump to 4.19.6
kernel: V4L2 codec interface
See: raspberrypi/linux#2755

kernel: Add support for SLB9760 Iridium / LetsTrust TPM boards
See: raspberrypi/linux#2585

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Dec 4, 2018

kernel: Bump to 4.14.85
kernel: Add support for SLB9760 Iridium / LetsTrust TPM boards
See: raspberrypi/linux#2585

kernel: staging: bcm2835-camera: Fix stride on RGB3/BGR3 formats
See: raspberrypi/linux#2773

firmware: dispmanx: Also apply overscan_scale when clamping to screen
See: https://forum.kodi.tv/showthread.php?tid=338052

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Dec 4, 2018

kernel: Bump to 4.14.85
kernel: Add support for SLB9760 Iridium / LetsTrust TPM boards
See: raspberrypi/linux#2585

kernel: staging: bcm2835-camera: Fix stride on RGB3/BGR3 formats
See: raspberrypi/linux#2773

firmware: dispmanx: Also apply overscan_scale when clamping to screen
See: https://forum.kodi.tv/showthread.php?tid=338052

@PeterHuewe PeterHuewe deleted the PeterHuewe:tpm_tis_spi_rpi branch Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment