[PATCH RFC] tpm: define a command filter

[PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

This commit adds a command filter for whitelisting a set of commands in
a TPM space. When a TPM space is created through /dev/tpms0, no
commands are allowed. The user of the TPM space must explicitly define
the list of commands allowed before sending any commands. This ioctl is
a one shot call so that a resource manager daemon can call it before
sending the file descriptor to the client.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
1. This patch applies on top of 'tabrm4' brach.
2. Only compilation is tested (just drafted the idea)
 drivers/char/tpm/tpm-interface.c | 12 +++++--
 drivers/char/tpm/tpm.h           |  1 +
 drivers/char/tpm/tpm2-space.c    |  7 ++++
 drivers/char/tpm/tpms-dev.c      | 75 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/tpms.h        | 29 ++++++++++++++++
 5 files changed, 121 insertions(+), 3 deletions(-)
 create mode 100644 include/uapi/linux/tpms.h

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 70ee347..e0d9019 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -328,8 +328,8 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd,
-				 size_t len)
+static bool tpm_validate_command(struct tpm_chip *chip, struct tpm_space *space,
+				 const u8 *cmd, size_t len)
 {
 	const struct tpm_input_header *header = (const void *)cmd;
 	int i;
@@ -350,6 +350,12 @@ static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd,
 			return false;
 		}
 
+		if (!test_bit(i, space->cmd_filter)) {
+			dev_dbg(&chip->dev, "0x%04X is not allowed command\n",
+				cc);
+			return false;
+		}
+
 		attrs = chip->cc_attrs_tbl[i];
 		nr_handles = 4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0));
 		if (len < TPM_HEADER_SIZE + 4 * nr_handles)
@@ -383,7 +389,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	u32 count, ordinal;
 	unsigned long stop;
 
-	if (!tpm_validate_command(chip, buf, bufsiz))
+	if (!tpm_validate_command(chip, space, buf, bufsiz))
 		return -EINVAL;
 
 	if (bufsiz > TPM_BUFSIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c48255e..775bdf5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -159,6 +159,7 @@ enum tpm2_cc_attrs {
 struct tpm_space {
 	u32 context_tbl[3];
 	u8 *context_buf;
+	unsigned long *cmd_filter;
 };
 
 enum tpm_chip_flags {
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 5d23466..081ab03 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -38,6 +38,13 @@ int tpm2_init_space(struct tpm_chip *chip, struct tpm_space *space)
 	if (!space->context_buf)
 		return -ENOMEM;
 
+	space->cmd_filter = kzalloc(BITS_TO_LONGS(chip->nr_commands),
+				    GFP_KERNEL);
+	if (!space->cmd_filter) {
+		kfree(space->context_buf);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
index 2ac2537..a610368 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -4,11 +4,13 @@
  * GPLv2
  */
 #include <linux/slab.h>
+#include <uapi/linux/tpms.h>
 #include "tpm-dev.h"
 
 struct tpms_priv {
 	struct file_priv priv;
 	struct tpm_space space;
+	unsigned long flags;
 };
 
 static int tpms_open(struct inode *inode, struct file *file)
@@ -40,6 +42,7 @@ static int tpms_release(struct inode *inode, struct file *file)
 
 	tpm_common_release(file, fpriv);
 	kfree(priv->space.context_buf);
+	kfree(priv->space.cmd_filter);
 	kfree(priv);
 
 	return 0;
@@ -54,12 +57,84 @@ ssize_t tpms_write(struct file *file, const char __user *buf,
 	return tpm_common_write(file, buf, size, off, &priv->space);
 }
 
+/**
+ * tpm_ioc_set_command_filter - handler for %SGX_IOC_SET_COMMAND_FILTER ioctl
+ *
+ * Takes a list of command codes in the form of array of 16-bit words as input.
+ * This defines the set of command allowed to be used in the associated TPM
+ * space. This is a one shot call: an RM daemon can set the filter and client
+ * cannot increase its privileges afterwards.
+ */
+static long tpms_ioc_set_command_filter(struct file *file, unsigned int ioctl,
+					unsigned long arg)
+{
+	struct tpms_priv *priv = file->private_data;
+	struct tpm_chip *chip = priv->priv.chip;
+	struct tpms_command_filter cmd_filter;
+	u16 *commands;
+	int i;
+	int j;
+
+	/* one shot */
+	if (test_and_set_bit(0, &priv->flags))
+		return -EFAULT;
+
+	if (copy_from_user(&cmd_filter, (void __user *)arg,
+			   sizeof(cmd_filter)))
+		return -EFAULT;
+
+	commands = kzalloc(2 * cmd_filter.nr_commands, GFP_KERNEL);
+	if (!commands)
+		return -ENOMEM;
+
+	if (copy_from_user(commands, (void __user *)cmd_filter.commands,
+			   2 * cmd_filter.nr_commands)) {
+		kfree(commands);
+		return -EFAULT;
+	}
+
+	for (i = 0; i < cmd_filter.nr_commands; i++) {
+		j = tpm2_find_cc(chip, commands[i]);
+		if (j < 0) {
+			kfree(commands);
+			return -EINVAL;
+		}
+
+		set_bit(j, priv->space.cmd_filter);
+	}
+
+	kfree(commands);
+	return 0;
+}
+
+static long tpms_ioctl(struct file *file, unsigned int ioctl,
+		       unsigned long arg)
+{
+	switch (ioctl) {
+	case TPMS_IOC_SET_COMMAND_FILTER:
+		tpms_ioc_set_command_filter(file, ioctl, arg);
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static long tpms_compat_ioctl(struct file *file, unsigned int ioctl,
+			     unsigned long arg)
+{
+	return tpms_ioctl(file, ioctl, arg);
+}
+#endif
 const struct file_operations tpms_fops = {
 	.owner = THIS_MODULE,
 	.llseek = no_llseek,
 	.open = tpms_open,
 	.read = tpm_common_read,
 	.write = tpms_write,
+	.unlocked_ioctl = tpms_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = tpms_compat_ioctl,
+#endif
 	.release = tpms_release,
 };
 
diff --git a/include/uapi/linux/tpms.h b/include/uapi/linux/tpms.h
new file mode 100644
index 0000000..3a86e05
--- /dev/null
+++ b/include/uapi/linux/tpms.h
@@ -0,0 +1,29 @@
+/*
+ * API and definitions for the TPM device driver
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Authors:
+ * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#ifndef _UAPI_TPMS_H
+#define _UAPI_TPMS_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define TPMS_IOC_MAGIC 0xa2
+#define TPMS_IOC_SET_COMMAND_FILTER \
+	_IOW(TPMS_IOC_MAGIC, 0x00, struct tpms_command_filter)
+
+struct tpms_command_filter {
+	__u32	nr_commands;
+	__u64	commands;
+};
+
+#endif /* _UAPI_TPMS_H */
-- 
2.9.3

Re: [PATCH RFC] tpm: define a command filter

[Jason Gunthorpe]

On Tue, Jan 24, 2017 at 02:02:52AM +0200, Jarkko Sakkinen wrote:
> This commit adds a command filter for whitelisting a set of commands in
> a TPM space. When a TPM space is created through /dev/tpms0, no
> commands are allowed. The user of the TPM space must explicitly define
> the list of commands allowed before sending any commands. This ioctl is
> a one shot call so that a resource manager daemon can call it before
> sending the file descriptor to the client.

I don't think it makes sense to have a daemon in user space that
passes an open'd /dev/tpms0 FD directly to a client..

It is trivial and more powerful to just proxy the messages. Can you
see some reason why passing a FD through a daemon would make sense?

The earlier discussion with James was to have some way to apply a
global command filter to all tpms0 users with the idea that the
'right' restricted command set would enable a 0666 cdev node, and no
daemon.

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 1. This patch applies on top of 'tabrm4' brach.
> 2. Only compilation is tested (just drafted the idea)
>  drivers/char/tpm/tpm-interface.c | 12 +++++--
>  drivers/char/tpm/tpm.h           |  1 +
>  drivers/char/tpm/tpm2-space.c    |  7 ++++
>  drivers/char/tpm/tpms-dev.c      | 75 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/tpms.h        | 29 ++++++++++++++++

BTW, don't forget to update kbuild when you add uapi files... Applies
to other patches..

Jason

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Mon, Jan 23, 2017 at 05:19:18PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 24, 2017 at 02:02:52AM +0200, Jarkko Sakkinen wrote:
> > This commit adds a command filter for whitelisting a set of commands in
> > a TPM space. When a TPM space is created through /dev/tpms0, no
> > commands are allowed. The user of the TPM space must explicitly define
> > the list of commands allowed before sending any commands. This ioctl is
> > a one shot call so that a resource manager daemon can call it before
> > sending the file descriptor to the client.
> 
> I don't think it makes sense to have a daemon in user space that
> passes an open'd /dev/tpms0 FD directly to a client..
> 
> It is trivial and more powerful to just proxy the messages. Can you
> see some reason why passing a FD through a daemon would make sense?
> 
> The earlier discussion with James was to have some way to apply a
> global command filter to all tpms0 users with the idea that the
> 'right' restricted command set would enable a 0666 cdev node, and no
> daemon.

Is that a conflicting goal?

Maybe the ioctl could be restricted by CAP_MAC_ADMIN in that case?

How would you propose to change the code below? I guess the "core
code" is about right and this is more about API, am I right?

/Jarkko

> 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 1. This patch applies on top of 'tabrm4' brach.
> > 2. Only compilation is tested (just drafted the idea)
> >  drivers/char/tpm/tpm-interface.c | 12 +++++--
> >  drivers/char/tpm/tpm.h           |  1 +
> >  drivers/char/tpm/tpm2-space.c    |  7 ++++
> >  drivers/char/tpm/tpms-dev.c      | 75 ++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/tpms.h        | 29 ++++++++++++++++
> 
> BTW, don't forget to update kbuild when you add uapi files... Applies
> to other patches..
> 
> Jason

Re: [PATCH RFC] tpm: define a command filter

[Jason Gunthorpe]

On Tue, Jan 24, 2017 at 04:36:00PM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 05:19:18PM -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 24, 2017 at 02:02:52AM +0200, Jarkko Sakkinen wrote:
> > > This commit adds a command filter for whitelisting a set of commands in
> > > a TPM space. When a TPM space is created through /dev/tpms0, no
> > > commands are allowed. The user of the TPM space must explicitly define
> > > the list of commands allowed before sending any commands. This ioctl is
> > > a one shot call so that a resource manager daemon can call it before
> > > sending the file descriptor to the client.
> > 
> > I don't think it makes sense to have a daemon in user space that
> > passes an open'd /dev/tpms0 FD directly to a client..
> > 
> > It is trivial and more powerful to just proxy the messages. Can you
> > see some reason why passing a FD through a daemon would make sense?
> > 
> > The earlier discussion with James was to have some way to apply a
> > global command filter to all tpms0 users with the idea that the
> > 'right' restricted command set would enable a 0666 cdev node, and no
> > daemon.
> 
> Is that a conflicting goal?
>
> Maybe the ioctl could be restricted by CAP_MAC_ADMIN in that case?

I think you need to spell out a clear use case for how userspace
should use this filter feature and why having the kernel involved is a
necessary element.

Driving userspace from the kernel uAPI design is a bit tricky without
participation from people writing the user space code.

> How would you propose to change the code below? I guess the "core
> code" is about right and this is more about API, am I right?

Generally, I'm of the opinion it is better to introduce the minimal
amount of uAPI at this point and wait until people working on
userspace figure out basic questions like, will there be a TPM2 daemon
or not..

I would focus now on getting the RFC series finished up, hook the
kAPI users into spaces and get it to the point where it does let
user & kernel safely share the TPM.

Jason

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Tue, Jan 24, 2017 at 12:07:07PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 24, 2017 at 04:36:00PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 05:19:18PM -0700, Jason Gunthorpe wrote:
> > > On Tue, Jan 24, 2017 at 02:02:52AM +0200, Jarkko Sakkinen wrote:
> > > > This commit adds a command filter for whitelisting a set of commands in
> > > > a TPM space. When a TPM space is created through /dev/tpms0, no
> > > > commands are allowed. The user of the TPM space must explicitly define
> > > > the list of commands allowed before sending any commands. This ioctl is
> > > > a one shot call so that a resource manager daemon can call it before
> > > > sending the file descriptor to the client.
> > > 
> > > I don't think it makes sense to have a daemon in user space that
> > > passes an open'd /dev/tpms0 FD directly to a client..
> > > 
> > > It is trivial and more powerful to just proxy the messages. Can you
> > > see some reason why passing a FD through a daemon would make sense?
> > > 
> > > The earlier discussion with James was to have some way to apply a
> > > global command filter to all tpms0 users with the idea that the
> > > 'right' restricted command set would enable a 0666 cdev node, and no
> > > daemon.
> > 
> > Is that a conflicting goal?
> >
> > Maybe the ioctl could be restricted by CAP_MAC_ADMIN in that case?
> 
> I think you need to spell out a clear use case for how userspace
> should use this filter feature and why having the kernel involved is a
> necessary element.
> 
> Driving userspace from the kernel uAPI design is a bit tricky without
> participation from people writing the user space code.
> 
> > How would you propose to change the code below? I guess the "core
> > code" is about right and this is more about API, am I right?
> 
> Generally, I'm of the opinion it is better to introduce the minimal
> amount of uAPI at this point and wait until people working on
> userspace figure out basic questions like, will there be a TPM2 daemon
> or not..
> 
> I would focus now on getting the RFC series finished up, hook the
> kAPI users into spaces and get it to the point where it does let
> user & kernel safely share the TPM.
> 
> Jason

There should be anyway someway to limit what commands can be sent but
I understand your point.

Would it make more sense to have a sysfs file for configuring the
global filter that would get the data in the same format (list of
16-bit words)?

/Jarkko

Re: [PATCH RFC] tpm: define a command filter

[Jason Gunthorpe]

On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:

> There should be anyway someway to limit what commands can be sent but
> I understand your point.

What is the filter for?

James and I talked about a filter to create a safer cdev for use by
users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
access' path.

I also suggested a filter in the kernel to ensure that the RM is only
passing commands it actually knows it handles properly. eg you would
filter out list handles. That is hardwired into the kernel, and does
not ge to be configured by user space.

> Would it make more sense to have a sysfs file for configuring the
> global filter that would get the data in the same format (list of
> 16-bit words)?

Probably not, then there is no way to escape the filter in userspace,
so some command just become impossible even for root. (And no,
something like tpm should not test CAP_ flags, that is putting
too much policy into the kernel)

Jason

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Wed, Jan 25, 2017 at 03:11:36PM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:
> 
> > There should be anyway someway to limit what commands can be sent but
> > I understand your point.
> 
> What is the filter for?
> 
> James and I talked about a filter to create a safer cdev for use by
> users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
> access' path.

What do you mean by "safer cdev"?

> I also suggested a filter in the kernel to ensure that the RM is only
> passing commands it actually knows it handles properly. eg you would
> filter out list handles. That is hardwired into the kernel, and does
> not ge to be configured by user space.

In many cases you would want to limit the set of operations that client
can use.  For example, not every client needs NV operations. In general
you might want to have mechanism for limiting privileges. I haven't
really considered this from the perspective that you've been discussing
but more from the "principle of least privilege" perspective.

Are you suggesting that in such cases you could just create daemon for
proxying the traffic (when you want to limit privileges)? You could just
make that daemon a whole lot simpler if it just needs to pass the file
desriptor to the client after defining the set of operations that the
client can use.

This is a high priority decision to make because it's hard to apply
principle of least privilege (with everything disallowed defaultts)
if it is not done in the first place.

/Jarkko

Re: [PATCH RFC] tpm: define a command filter

[Jason Gunthorpe]

On Thu, Jan 26, 2017 at 01:14:03PM +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 25, 2017 at 03:11:36PM -0700, Jason Gunthorpe wrote:
> > On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:
> > 
> > > There should be anyway someway to limit what commands can be sent but
> > > I understand your point.
> > 
> > What is the filter for?
> > 
> > James and I talked about a filter to create a safer cdev for use by
> > users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
> > access' path.
> 
> What do you mean by "safer cdev"?

'safer cdev' is this concept of limiting privileges you are describing
below.

> > I also suggested a filter in the kernel to ensure that the RM is only
> > passing commands it actually knows it handles properly. eg you would
> > filter out list handles. That is hardwired into the kernel, and does
> > not ge to be configured by user space.
>
> In many cases you would want to limit the set of operations that client
> can use.  For example, not every client needs NV operations. In general
> you might want to have mechanism for limiting privileges. I haven't
> really considered this from the perspective that you've been discussing
> but more from the "principle of least privilege" perspective.

What does that mean? The kernel needs to provide an unrestricted
access path to the TPM and the RM - typically for use by root. I don't
think there is any debate on this point.

The kernel *could* provide restricted access to the TPM and the RM -
typically for use by a user.

These are *different* things and they should not both exist at once on
/dev/tpms0 (that is not the unix model).

IMHO this patch series should focus entirely on the unrestricted
access path. Otherwise the debate is too large and complex.

> Are you suggesting that in such cases you could just create daemon for
> proxying the traffic (when you want to limit privileges)?

If a daemon is to be involved in userspace then yes, it should proxy
the traffic and not use fd passing. That is easier and more normal.

Eg a daemon that creates /run/tpm.socket could present the usual
read/write interface and instantly provide the compatability that
James was talking about.

I would *like* to see a daemon-less solution, but to me that means a
kernel cdev that defaults to 0666 permissions. Until a patch comes
along that does that I propose people experiment using a user
space proxy... Mostly out of fear that we create a security issue in
the kernel by rushing.

> You could just make that daemon a whole lot simpler if it just needs
> to pass the file desriptor to the client after defining the set of
> operations that the client can use.

That really isn't simpler :)

Proxying + filtering is very easy as long as there is a 1:1
relationship of client and kernel RM fd. Doing all the filtering in
userspace is *clearly* better than trying to split it into the kernel.

fd passing is pretty arcane and doesn't bring any benifit here, you
are trying to be too clever.

> This is a high priority decision to make because it's hard to apply
> principle of least privilege (with everything disallowed defaultts)
> if it is not done in the first place.

This is why I argued these patches should use ioctl and defer a new
cdev until we sort the question of a restricted kernel interface. tpm0
is already the unrestricted access path to the TPM, it makes sense to
continue to use it in that role for the RM.

Otherwise tpms0 becomes the unrestricted interface to the RM and we
might need /dev/tpm_restricted0 down the road, which I think is really
ugly.

Jason

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 11:05:06AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 01:14:03PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 25, 2017 at 03:11:36PM -0700, Jason Gunthorpe wrote:
> > > On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > There should be anyway someway to limit what commands can be sent but
> > > > I understand your point.
> > > 
> > > What is the filter for?
> > > 
> > > James and I talked about a filter to create a safer cdev for use by
> > > users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
> > > access' path.
> > 
> > What do you mean by "safer cdev"?
> 
> 'safer cdev' is this concept of limiting privileges you are describing
> below.
> 
> > > I also suggested a filter in the kernel to ensure that the RM is only
> > > passing commands it actually knows it handles properly. eg you would
> > > filter out list handles. That is hardwired into the kernel, and does
> > > not ge to be configured by user space.
> >
> > In many cases you would want to limit the set of operations that client
> > can use.  For example, not every client needs NV operations. In general
> > you might want to have mechanism for limiting privileges. I haven't
> > really considered this from the perspective that you've been discussing
> > but more from the "principle of least privilege" perspective.
> 
> What does that mean? The kernel needs to provide an unrestricted
> access path to the TPM and the RM - typically for use by root. I don't
> think there is any debate on this point.
> 
> The kernel *could* provide restricted access to the TPM and the RM -
> typically for use by a user.
> 
> These are *different* things and they should not both exist at once on
> /dev/tpms0 (that is not the unix model).
> 
> IMHO this patch series should focus entirely on the unrestricted
> access path. Otherwise the debate is too large and complex.

Agreed. We can add more granular access control later on.

For the rest of the response I understand your point of view but lets
continue after we have basic building blocks in place :-)

/Jarkko