[PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Stefan Berger]

Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
instance from having the typical sysfs entries that shows the state of the
TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.

This flag would be set by a container management stack to prevent
TPM related information leakage between containers who share entries
in sysfs.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-chip.c       | 2 +-
 drivers/char/tpm/tpm.h            | 1 +
 drivers/char/tpm/tpm_vtpm_proxy.c | 5 ++++-
 include/uapi/linux/vtpm_proxy.h   | 3 ++-
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1965dc7..c8e7cab 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -278,7 +278,7 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 static int tpm1_chip_register(struct tpm_chip *chip)
 {
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+	if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_NO_SYSFS))
 		return 0;
 
 	tpm_sysfs_add_device(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8890df2..9d8cf81 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -143,6 +143,7 @@ enum tpm_chip_flags {
 	TPM_CHIP_FLAG_TPM2		= BIT(1),
 	TPM_CHIP_FLAG_IRQ		= BIT(2),
 	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
+	TPM_CHIP_FLAG_NO_SYSFS		= BIT(4),
 };
 
 struct tpm_chip {
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 86e27e8..81079e0 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -50,7 +50,7 @@ struct proxy_dev {
 };
 
 /* all supported flags */
-#define VTPM_PROXY_FLAGS_ALL  (VTPM_PROXY_FLAG_TPM2)
+#define VTPM_PROXY_FLAGS_ALL  (VTPM_PROXY_FLAG_TPM2 | VTPM_PROXY_FLAG_NO_SYSFS)
 
 static struct workqueue_struct *workqueue;
 
@@ -490,6 +490,9 @@ static struct file *vtpm_proxy_create_device(
 	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
 		proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;
 
+	if (proxy_dev->flags & VTPM_PROXY_FLAG_NO_SYSFS)
+		proxy_dev->chip->flags |= TPM_CHIP_FLAG_NO_SYSFS;
+
 	vtpm_proxy_work_start(proxy_dev);
 
 	vtpm_new_dev->fd = fd;
diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
index 41e8e22..151ea48 100644
--- a/include/uapi/linux/vtpm_proxy.h
+++ b/include/uapi/linux/vtpm_proxy.h
@@ -29,7 +29,8 @@ struct vtpm_proxy_new_dev {
 };
 
 /* above flags */
-#define VTPM_PROXY_FLAG_TPM2  1  /* emulator is TPM 2 */
+#define VTPM_PROXY_FLAG_TPM2      1  /* emulator is TPM 2 */
+#define VTPM_PROXY_FLAG_NO_SYSFS  2  /* no sysfs for vTPM proxy instance */
 
 #define VTPM_PROXY_IOC_NEW_DEV   _IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
 
-- 
2.4.3

Re: [PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Jason Gunthorpe]

On Fri, Jun 24, 2016 at 10:36:55AM -0400, Stefan Berger wrote:
> Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
> instance from having the typical sysfs entries that shows the state of the
> TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
> pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.

No other subsystem does something so goofy, this really needs to be
part of namespace support for TPM.

Why can't you just make the sysfs files unreadable in user space?
If a container can make them readable again can't it also just create
the chardev node?

Jason

Re: [PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Stefan Berger]

On 06/24/2016 01:48 PM, Jason Gunthorpe wrote:
> On Fri, Jun 24, 2016 at 10:36:55AM -0400, Stefan Berger wrote:
>> Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
>> instance from having the typical sysfs entries that shows the state of the
>> TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
>> pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.
> No other subsystem does something so goofy, this really needs to be
> part of namespace support for TPM.

And I am not sure how to go about this. TPM2 by the way doesn't have 
such entries, so it's much better from that perspective.

>
> Why can't you just make the sysfs files unreadable in user space?

There are actually ways to go about this. Likely bind-mounting over 
/sys/device/virtual/tpm would be one solution to hide all virtual TPM 
device. Another is applying an AppArmor policy to the container denying 
access to tpm directories or entries. SELinux would not be so easy.

The flag in this patch seemed like a 'cheap' way to eliminate that 
problem as well.

> If a container can make them readable again can't it also just create
> the chardev node?

What do you mean by making them readable again? The chardev node can be 
created inside the container with the major and minor numbers as it was 
created with the ioctl.

     Stefan


>
> Jason
>

Re: [PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Jason Gunthorpe]

On Fri, Jun 24, 2016 at 02:43:00PM -0400, Stefan Berger wrote:
> On 06/24/2016 01:48 PM, Jason Gunthorpe wrote:
> >On Fri, Jun 24, 2016 at 10:36:55AM -0400, Stefan Berger wrote:
> >>Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
> >>instance from having the typical sysfs entries that shows the state of the
> >>TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
> >>pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.
> >No other subsystem does something so goofy, this really needs to be
> >part of namespace support for TPM.
> 
> And I am not sure how to go about this. TPM2 by the way doesn't have such
> entries, so it's much better from that perspective.

Presumably this will become obvious when you figure out how to make
namespaces work for IMA.. tpm #0 will have to change, and that will
have to impact the sysfs layout.

> >Why can't you just make the sysfs files unreadable in user space?
> 
> There are actually ways to go about this. Likely bind-mounting over
> /sys/device/virtual/tpm would be one solution to hide all virtual TPM
> device. Another is applying an AppArmor policy to the container denying
> access to tpm directories or entries. SELinux would not be so easy.

Seems reasonable

> The flag in this patch seemed like a 'cheap' way to eliminate that
> problem as well.

Generally hacks like this are discouraged in the kernel.

> >If a container can make them readable again can't it also just create
> >the chardev node?
> 
> What do you mean by making them readable again? The chardev node can be
> created inside the container with the major and minor numbers as it was
> created with the ioctl.

Well, root in the container can undo all sorts of things. It can
unbind mount the sysfs, it could change file permissions and it could
manually create the char dev nodes. AFAIK the only way to protect
against root in the container is to have full kernel namespace support
for the device..

Jason

Re: [PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Jarkko Sakkinen]

On Fri, Jun 24, 2016 at 02:43:00PM -0400, Stefan Berger wrote:
> On 06/24/2016 01:48 PM, Jason Gunthorpe wrote:
> >On Fri, Jun 24, 2016 at 10:36:55AM -0400, Stefan Berger wrote:
> >>Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
> >>instance from having the typical sysfs entries that shows the state of the
> >>TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
> >>pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.
> >No other subsystem does something so goofy, this really needs to be
> >part of namespace support for TPM.
> 
> And I am not sure how to go about this. TPM2 by the way doesn't have such
> entries, so it's much better from that perspective.
> 
> >
> >Why can't you just make the sysfs files unreadable in user space?
> 
> There are actually ways to go about this. Likely bind-mounting over
> /sys/device/virtual/tpm would be one solution to hide all virtual TPM
> device. Another is applying an AppArmor policy to the container denying
> access to tpm directories or entries. SELinux would not be so easy.
> 
> The flag in this patch seemed like a 'cheap' way to eliminate that problem
> as well.

Does it have any other qualities that would make this better than bind
mounting?

/Jarkko

Re: [PATCH] tpm: vtpm_proxy: Introduce flag to prevent sysfs entries

[Stefan Berger]

On 06/27/2016 02:32 PM, Jarkko Sakkinen wrote:
> On Fri, Jun 24, 2016 at 02:43:00PM -0400, Stefan Berger wrote:
>> On 06/24/2016 01:48 PM, Jason Gunthorpe wrote:
>>> On Fri, Jun 24, 2016 at 10:36:55AM -0400, Stefan Berger wrote:
>>>> Introduce TPM_VTPM_PROXY_NO_SYSFS flag that prevents a vtpm_proxy driver
>>>> instance from having the typical sysfs entries that shows the state of the
>>>> TPM. The flag is to be set in the ioctl creating the vtpm_proxy device
>>>> pair and maps on a new chip flags TPM_CHIP_FLAG_NO_SYSFS.
>>> No other subsystem does something so goofy, this really needs to be
>>> part of namespace support for TPM.
>> And I am not sure how to go about this. TPM2 by the way doesn't have such
>> entries, so it's much better from that perspective.
>>
>>> Why can't you just make the sysfs files unreadable in user space?
>> There are actually ways to go about this. Likely bind-mounting over
>> /sys/device/virtual/tpm would be one solution to hide all virtual TPM
>> device. Another is applying an AppArmor policy to the container denying
>> access to tpm directories or entries. SELinux would not be so easy.
>>
>> The flag in this patch seemed like a 'cheap' way to eliminate that problem
>> as well.
> Does it have any other qualities that would make this better than bind
> mounting?

Not sure what to argue with. Maybe setting a bit on an ioctl that needs 
to be executed would be a good argument.

    stefan

>
> /Jarkko
>