diff mbox

[tpmdd-devel,v3,1/3] tpm: vtpm_proxy: Implement new ioctl to get supported flags

Message ID 1493909787-1848-2-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger May 4, 2017, 2:56 p.m. UTC
Implement VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl to get the bitmask
of flags that the vtpm_proxy driver supports in the
VTPM_PROXY_IOC_NEW_DEV ioctl. This helps user space in deciding
which flags to set in that ioctl.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_vtpm_proxy.c | 29 +++++++++++++++++++++++++++++
 include/uapi/linux/vtpm_proxy.h   | 11 +++++++++++
 2 files changed, 40 insertions(+)

Comments

Jason Gunthorpe May 4, 2017, 3:34 p.m. UTC | #1
On Thu, May 04, 2017 at 10:56:25AM -0400, Stefan Berger wrote:
> Implement VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl to get the bitmask
> of flags that the vtpm_proxy driver supports in the
> VTPM_PROXY_IOC_NEW_DEV ioctl. This helps user space in deciding
> which flags to set in that ioctl.

you might be better off just having a VTPM_PROXY_IO_ENABLE_FEATURE
.feature = LOCALITY

If that fails then the feature is not supported, no real need for the
query in that case.

Not sure about Jarkko's point on request/release locality.. Is there a
scenario where the emulator should fail the request locality?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Stefan Berger May 4, 2017, 5:13 p.m. UTC | #2
On 05/04/2017 11:34 AM, Jason Gunthorpe wrote:
> On Thu, May 04, 2017 at 10:56:25AM -0400, Stefan Berger wrote:
>> Implement VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl to get the bitmask
>> of flags that the vtpm_proxy driver supports in the
>> VTPM_PROXY_IOC_NEW_DEV ioctl. This helps user space in deciding
>> which flags to set in that ioctl.
> you might be better off just having a VTPM_PROXY_IO_ENABLE_FEATURE
> .feature = LOCALITY

Do you have an example driver that shows how to do this ? Can user space 
query that feature?

>
> If that fails then the feature is not supported, no real need for the
> query in that case.
>
> Not sure about Jarkko's point on request/release locality.. Is there a
> scenario where the emulator should fail the request locality?

We could filter localities 5 and higher on the level of the driver 
(patch 2/3) since basically there are only 5 localities (0-4) in any TPM 
interface today. The typical hardware locality 4 would be filtered by 
the emulator per policy passed via command line, but I would allow it on 
the level of this driver. An error message would be returned for any 
command executed in that locality, unless the 'policy' allows it. 
Localities 0-3 should just be selectable. The TPM TIS (in the hardware) 
implements some complicated scheme when it comes to allowing the 
selection of a locality and I would say we need none of that but just 
tell the vTPM proxy driver the locality (patch 2/3) in which the next 
command will be executed.


>
> Jason
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jason Gunthorpe May 4, 2017, 5:20 p.m. UTC | #3
On Thu, May 04, 2017 at 01:13:18PM -0400, Stefan Berger wrote:
> On 05/04/2017 11:34 AM, Jason Gunthorpe wrote:
> >On Thu, May 04, 2017 at 10:56:25AM -0400, Stefan Berger wrote:
> >>Implement VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl to get the bitmask
> >>of flags that the vtpm_proxy driver supports in the
> >>VTPM_PROXY_IOC_NEW_DEV ioctl. This helps user space in deciding
> >>which flags to set in that ioctl.
> >you might be better off just having a VTPM_PROXY_IO_ENABLE_FEATURE
> >.feature = LOCALITY
> 
> Do you have an example driver that shows how to do this ? Can user space
> query that feature?

Try and enable the feature, if it fails then there is no feature in
the kernel.

This is the usual way to add new syscalls..

> >If that fails then the feature is not supported, no real need for the
> >query in that case.
> >
> >Not sure about Jarkko's point on request/release locality.. Is there a
> >scenario where the emulator should fail the request locality?
> 
> We could filter localities 5 and higher on the level of the driver (patch
> 2/3) since basically there are only 5 localities (0-4) in any TPM interface
> today. The typical hardware locality 4 would be filtered by the emulator per
> policy passed via command line, but I would allow it on the level of this
> driver. An error message would be returned for any command executed in that
> locality, unless the 'policy' allows it. Localities 0-3 should just be
> selectable. The TPM TIS (in the hardware) implements some complicated scheme
> when it comes to allowing the selection of a locality and I would say we
> need none of that but just tell the vTPM proxy driver the locality (patch
> 2/3) in which the next command will be executed.

Well, if TIS hardware has some scheme I feel like the emulator uAPI should
have enough fidelity to ecompass existing hardware, even if your
current emulator does not need it.

So allowing request_locality to fail from userspace seems reasonable.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Stefan Berger May 4, 2017, 5:28 p.m. UTC | #4
On 05/04/2017 01:20 PM, Jason Gunthorpe wrote:
> On Thu, May 04, 2017 at 01:13:18PM -0400, Stefan Berger wrote:
>> On 05/04/2017 11:34 AM, Jason Gunthorpe wrote:
>>> On Thu, May 04, 2017 at 10:56:25AM -0400, Stefan Berger wrote:
>>>> Implement VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl to get the bitmask
>>>> of flags that the vtpm_proxy driver supports in the
>>>> VTPM_PROXY_IOC_NEW_DEV ioctl. This helps user space in deciding
>>>> which flags to set in that ioctl.
>>> you might be better off just having a VTPM_PROXY_IO_ENABLE_FEATURE
>>> .feature = LOCALITY
>> Do you have an example driver that shows how to do this ? Can user space
>> query that feature?
> Try and enable the feature, if it fails then there is no feature in
> the kernel.
>
> This is the usual way to add new syscalls..
>
>>> If that fails then the feature is not supported, no real need for the
>>> query in that case.
>>>
>>> Not sure about Jarkko's point on request/release locality.. Is there a
>>> scenario where the emulator should fail the request locality?
>> We could filter localities 5 and higher on the level of the driver (patch
>> 2/3) since basically there are only 5 localities (0-4) in any TPM interface
>> today. The typical hardware locality 4 would be filtered by the emulator per
>> policy passed via command line, but I would allow it on the level of this
>> driver. An error message would be returned for any command executed in that
>> locality, unless the 'policy' allows it. Localities 0-3 should just be
>> selectable. The TPM TIS (in the hardware) implements some complicated scheme
>> when it comes to allowing the selection of a locality and I would say we
>> need none of that but just tell the vTPM proxy driver the locality (patch
>> 2/3) in which the next command will be executed.
> Well, if TIS hardware has some scheme I feel like the emulator uAPI should
> have enough fidelity to ecompass existing hardware, even if your
> current emulator does not need it.
>
> So allowing request_locality to fail from userspace seems reasonable.

What's the best interface to use for this ?



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jason Gunthorpe May 4, 2017, 5:31 p.m. UTC | #5
On Thu, May 04, 2017 at 01:28:17PM -0400, Stefan Berger wrote:

> >So allowing request_locality to fail from userspace seems reasonable.
> 
> What's the best interface to use for this ?

If locality support is enabled then send a request locality packet to
userspace and block until return, just like command execution?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Stefan Berger May 4, 2017, 5:33 p.m. UTC | #6
On 05/04/2017 01:31 PM, Jason Gunthorpe wrote:
> On Thu, May 04, 2017 at 01:28:17PM -0400, Stefan Berger wrote:
>
>>> So allowing request_locality to fail from userspace seems reasonable.
>> What's the best interface to use for this ?
> If locality support is enabled then send a request locality packet to
> userspace and block until return, just like command execution?

We would have to invent a command for that...



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 751059d..fb4d207 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -592,6 +592,33 @@  static long vtpmx_ioc_new_dev(struct file *file, unsigned int ioctl,
 	return 0;
 }
 
+/**
+ * vtpmx_ioc_get_supt_flags - handler for the %VTPM_PROXY_IOC_GET_SUPT_FLAGS
+ *                            ioctl
+ * @file:	/dev/vtpmx
+ * @ioctl:	the ioctl number
+ * @arg:	pointer to the struct vtpmx_proxy_get_supt_flags
+ *
+ * Return the bitfield of supported flags
+ */
+static long vtpmx_ioc_get_supt_flags(struct file *file, unsigned int ioctl,
+				     unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	struct vtpm_proxy_supt_flags __user *vtpm_supt_flags_p = argp;
+	struct vtpm_proxy_supt_flags flags = {
+		.flags = VTPM_PROXY_FLAGS_ALL,
+	};
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_to_user(vtpm_supt_flags_p, &flags, sizeof(flags)))
+		return -EFAULT;
+
+	return 0;
+}
+
 /*
  * vtpmx_fops_ioctl: ioctl on /dev/vtpmx
  *
@@ -604,6 +631,8 @@  static long vtpmx_fops_ioctl(struct file *f, unsigned int ioctl,
 	switch (ioctl) {
 	case VTPM_PROXY_IOC_NEW_DEV:
 		return vtpmx_ioc_new_dev(f, ioctl, arg);
+	case VTPM_PROXY_IOC_GET_SUPT_FLAGS:
+		return vtpmx_ioc_get_supt_flags(f, ioctl, arg);
 	default:
 		return -ENOIOCTLCMD;
 	}
diff --git a/include/uapi/linux/vtpm_proxy.h b/include/uapi/linux/vtpm_proxy.h
index a69e991..83e64e7 100644
--- a/include/uapi/linux/vtpm_proxy.h
+++ b/include/uapi/linux/vtpm_proxy.h
@@ -44,6 +44,17 @@  struct vtpm_proxy_new_dev {
 	__u32 minor;         /* output */
 };
 
+/**
+ * struct vtpm_proxy_supt_flags - parameter structure for the
+ *                                %VTPM_PROXY_IOC_GET_SUPT_FLAGS ioctl
+ * @flags: flags supported by the vtpm proxy driver
+ */
+struct vtpm_proxy_supt_flags {
+	__u32 flags;         /* output */
+};
+
 #define VTPM_PROXY_IOC_NEW_DEV	_IOWR(0xa1, 0x00, struct vtpm_proxy_new_dev)
+#define VTPM_PROXY_IOC_GET_SUPT_FLAGS \
+				_IOR(0xa1, 0x01, struct vtpm_proxy_supt_flags)
 
 #endif /* _UAPI_LINUX_VTPM_PROXY_H */