| Message ID | 20230530020133.235765-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] tpm: tpm_vtpm_proxy: do not reference kernel memory as user memory | expand |
On 5/29/23 22:01, Jarkko Sakkinen wrote: > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> > > The driver has two issues (in priority order) in the locality change: > > 1. The driver uses __user pointer and copy_to_user() and > copy_from_user() with a kernel address during the locality change. This can lead to ... bad things ? > 2. For invalid locality change request from user space, the driver > sets errno to EFAULT, while for invalid input data EINVAL should > be used. > > Address this by: > > 1. Introduce __vtpm_proxy_read_unlocked(), __vtpm_proxy_write_unlocked() > and __vtpm_proxy_read_write_locked(). > 2. Make locality change atomic by calling __vtpm_proxy_read_write_locked(), > instead of tpm_transmit_cmd(). 2. sounds like it should be addressable by changing -EFAULT to -EINVAL but there's more to it? > > Cc: stable@vger.kernel.org > Fixes: be4c9acfe297 ("tpm: vtpm_proxy: Implement request_locality function.") > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> > --- > v2: > * Acquiring and releasing mutex in-between should not be an issue > because they are executed with the chip locked. > --- > drivers/char/tpm/tpm_vtpm_proxy.c | 162 ++++++++++++++---------------- > 1 file changed, 73 insertions(+), 89 deletions(-) > > diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c > index 30e953988cab..8f43a82e5590 100644 > --- a/drivers/char/tpm/tpm_vtpm_proxy.c > +++ b/drivers/char/tpm/tpm_vtpm_proxy.c > @@ -38,7 +38,6 @@ struct proxy_dev { > #define STATE_OPENED_FLAG BIT(0) > #define STATE_WAIT_RESPONSE_FLAG BIT(1) /* waiting for emulator response */ > #define STATE_REGISTERED_FLAG BIT(2) > -#define STATE_DRIVER_COMMAND BIT(3) /* sending a driver specific command */ > > size_t req_len; /* length of queued TPM request */ > size_t resp_len; /* length of queued TPM response */ > @@ -58,106 +57,112 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev); > * Functions related to 'server side' > */ > > -/** > - * vtpm_proxy_fops_read - Read TPM commands on 'server side' > - * > - * @filp: file pointer > - * @buf: read buffer > - * @count: number of bytes to read > - * @off: offset > - * > - * Return: > - * Number of bytes read or negative error code > - */ > -static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf, > - size_t count, loff_t *off) > +static ssize_t __vtpm_proxy_read_unlocked(struct proxy_dev *proxy_dev, char __user *buf, > + size_t count) > { > - struct proxy_dev *proxy_dev = filp->private_data; > size_t len; > - int sig, rc; > - > - sig = wait_event_interruptible(proxy_dev->wq, > - proxy_dev->req_len != 0 || > - !(proxy_dev->state & STATE_OPENED_FLAG)); > - if (sig) > - return -EINTR; > - > - mutex_lock(&proxy_dev->buf_lock); > + int rc; > > - if (!(proxy_dev->state & STATE_OPENED_FLAG)) { > - mutex_unlock(&proxy_dev->buf_lock); > + if (!(proxy_dev->state & STATE_OPENED_FLAG)) > return -EPIPE; > - } > > len = proxy_dev->req_len; > > if (count < len || len > sizeof(proxy_dev->buffer)) { > - mutex_unlock(&proxy_dev->buf_lock); > pr_debug("Invalid size in recv: count=%zd, req_len=%zd\n", > count, len); > return -EIO; > } > > - rc = copy_to_user(buf, proxy_dev->buffer, len); > + if (buf) > + rc = copy_to_user(buf, proxy_dev->buffer, len); > + > memset(proxy_dev->buffer, 0, len); > proxy_dev->req_len = 0; > > if (!rc) > proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG; > > - mutex_unlock(&proxy_dev->buf_lock); > - > if (rc) > return -EFAULT; > > return len; > } > > -/** > - * vtpm_proxy_fops_write - Write TPM responses on 'server side' > - * > - * @filp: file pointer > - * @buf: write buffer > - * @count: number of bytes to write > - * @off: offset > - * > - * Return: > - * Number of bytes read or negative error value > - */ > -static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf, > - size_t count, loff_t *off) > +static ssize_t __vtpm_proxy_write_unlocked(struct proxy_dev *proxy_dev, const char __user *buf, > + size_t count) > { > - struct proxy_dev *proxy_dev = filp->private_data; > - > - mutex_lock(&proxy_dev->buf_lock); > - > - if (!(proxy_dev->state & STATE_OPENED_FLAG)) { > - mutex_unlock(&proxy_dev->buf_lock); > + if (!(proxy_dev->state & STATE_OPENED_FLAG)) > return -EPIPE; > - } > > if (count > sizeof(proxy_dev->buffer) || > - !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) { > - mutex_unlock(&proxy_dev->buf_lock); > + !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) > return -EIO; > - } > > proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG; > > proxy_dev->req_len = 0; > > - if (copy_from_user(proxy_dev->buffer, buf, count)) { > - mutex_unlock(&proxy_dev->buf_lock); > + if (buf && copy_from_user(proxy_dev->buffer, buf, count)) > return -EFAULT; > - } > > proxy_dev->resp_len = count; > + return count; > +} > > +static ssize_t __vtpm_proxy_read_write_unlocked(struct proxy_dev *proxy_dev, char __user *buf, > + size_t count) > +{ > + ssize_t rc; > + > + do { > + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count); > + if (rc < 0) > + break; > + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, rc); > + } while (0); > + > + return rc; > +} > + > +/* > + * See struct file_operations. > + */ > +static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf, > + size_t count, loff_t *off) > +{ > + struct proxy_dev *proxy_dev = filp->private_data; > + ssize_t rc; > + int sig; > + > + sig = wait_event_interruptible(proxy_dev->wq, > + proxy_dev->req_len != 0 || > + !(proxy_dev->state & STATE_OPENED_FLAG)); > + if (sig) > + return -EINTR; > + > + mutex_lock(&proxy_dev->buf_lock); > + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, count); > mutex_unlock(&proxy_dev->buf_lock); > > + return rc; > +} > + > +/* > + * See struct file_operations. > + */ > +static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf, > + size_t count, loff_t *off) > +{ > + struct proxy_dev *proxy_dev = filp->private_data; > + int rc; ssize_t rc; > + > + mutex_lock(&proxy_dev->buf_lock); > + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count); > + mutex_unlock(&proxy_dev->buf_lock); > wake_up_interruptible(&proxy_dev->wq); > > - return count; > + return rc; > } > > /* > @@ -295,28 +300,6 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) > return len; > } > > -static int vtpm_proxy_is_driver_command(struct tpm_chip *chip, > - u8 *buf, size_t count) > -{ > - struct tpm_header *hdr = (struct tpm_header *)buf; > - > - if (count < sizeof(struct tpm_header)) > - return 0; > - > - if (chip->flags & TPM_CHIP_FLAG_TPM2) { > - switch (be32_to_cpu(hdr->ordinal)) { > - case TPM2_CC_SET_LOCALITY: > - return 1; > - } > - } else { > - switch (be32_to_cpu(hdr->ordinal)) { > - case TPM_ORD_SET_LOCALITY: > - return 1; > - } > - } > - return 0; > -} > - > /* > * Called when core TPM driver forwards TPM requests to 'server side'. > * > @@ -330,6 +313,7 @@ static int vtpm_proxy_is_driver_command(struct tpm_chip *chip, > static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) > { > struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev); > + unsigned int ord = ((struct tpm_header *)buf)->ordinal; 'u32 ordinal' like in tpm_try_transmit ? you need this here in some form: be32_to_cpu(hdr->ordinal) > > if (count > sizeof(proxy_dev->buffer)) { > dev_err(&chip->dev,(chip->flags & TPM_CHIP_FLAG_TPM2) > @@ -338,9 +322,11 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) > return -EIO; > } > > - if (!(proxy_dev->state & STATE_DRIVER_COMMAND) && > - vtpm_proxy_is_driver_command(chip, buf, count)) > - return -EFAULT; > + if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY) > + return -EINVAL; > + > + if (ord == TPM_ORD_SET_LOCALITY) > + return -EINVAL; if ((chip->flags & TPM_CHIP_FLAG_TPM2)) { if (ord == ) return -EINVAL; } else if (ord == ) { return -EINVAL; } > > mutex_lock(&proxy_dev->buf_lock); > > @@ -409,12 +395,10 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality) > return rc; > tpm_buf_append_u8(&buf, locality); > > - proxy_dev->state |= STATE_DRIVER_COMMAND; > - > - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality"); > - > - proxy_dev->state &= ~STATE_DRIVER_COMMAND; > - > + mutex_lock(&proxy_dev->buf_lock); > + memcpy(proxy_dev->buffer, buf.data, tpm_buf_length(&buf)); > + rc = __vtpm_proxy_read_write_unlocked(proxy_dev, NULL, tpm_buf_length(&buf)); > + mutex_unlock(&proxy_dev->buf_lock); > if (rc < 0) { > locality = rc; > goto out; Stefan
On 5/29/23 22:01, Jarkko Sakkinen wrote: > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> > > - rc = copy_to_user(buf, proxy_dev->buffer, len); > + if (buf) > + rc = copy_to_user(buf, proxy_dev->buffer, len); > + Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read(). https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279 simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...); Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41
From: Stefan Berger > Sent: 30 May 2023 18:46 > > On 5/29/23 22:01, Jarkko Sakkinen wrote: > > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> > > > > > - rc = copy_to_user(buf, proxy_dev->buffer, len); > > + if (buf) > > + rc = copy_to_user(buf, proxy_dev->buffer, len); > > + > > Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on > file_operations.read(). If the user passes NULL the copy_to/from_user() fails and -EFAULT is returned. Adding the NULL check makes the request silently succeed. I doubt that is anywhere near right when you ignore copy_from_user(). I'm not sure what the rational/subject is about either. copy_to/from_user() calls access_ok() and will fail on a kernel address. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, 2023-05-30 at 13:45 -0400, Stefan Berger wrote: > > On 5/29/23 22:01, Jarkko Sakkinen wrote: > > From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> > > > > > - rc = copy_to_user(buf, proxy_dev->buffer, len); > > + if (buf) > > + rc = copy_to_user(buf, proxy_dev->buffer, len); > > + > > Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read(). > > > https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279 simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...); > > > Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41 It is good to mention here that IMA uses __user tagged pointers correctly, and it does not really compare to the vtpm driver code by any possible means. So let's not add illegit comparison points. BR, Jarkko
On 5/31/23 13:17, Jarkko Sakkinen wrote: > On Tue, 2023-05-30 at 13:45 -0400, Stefan Berger wrote: >> >> On 5/29/23 22:01, Jarkko Sakkinen wrote: >>> From: Jarkko Sakkinen <jarkko.sakkinen@tuni.fi> >>> >> >>> - rc = copy_to_user(buf, proxy_dev->buffer, len); >>> + if (buf) >>> + rc = copy_to_user(buf, proxy_dev->buffer, len); >>> + >> >> Looking through other drivers it seems buf is always expected to be a valid non-NULL pointer on file_operations.read(). >> >> >> https://elixir.bootlin.com/linux/latest/source/arch/x86/mm/tlb.c#L1279 simple_read_from_buffer will pass the pointer to the user buffer along and it ('to') ends up in copy_to_user(to, ...); >> >> >> Same here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L41 > > It is good to mention here that IMA uses __user tagged pointers > correctly, and it does not really compare to the vtpm driver code > by any possible means. So let's not add illegit comparison points. Yes, sir. Did you read David' response? > > BR, Jarkko
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c index 30e953988cab..8f43a82e5590 100644 --- a/drivers/char/tpm/tpm_vtpm_proxy.c +++ b/drivers/char/tpm/tpm_vtpm_proxy.c @@ -38,7 +38,6 @@ struct proxy_dev { #define STATE_OPENED_FLAG BIT(0) #define STATE_WAIT_RESPONSE_FLAG BIT(1) /* waiting for emulator response */ #define STATE_REGISTERED_FLAG BIT(2) -#define STATE_DRIVER_COMMAND BIT(3) /* sending a driver specific command */ size_t req_len; /* length of queued TPM request */ size_t resp_len; /* length of queued TPM response */ @@ -58,106 +57,112 @@ static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev); * Functions related to 'server side' */ -/** - * vtpm_proxy_fops_read - Read TPM commands on 'server side' - * - * @filp: file pointer - * @buf: read buffer - * @count: number of bytes to read - * @off: offset - * - * Return: - * Number of bytes read or negative error code - */ -static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf, - size_t count, loff_t *off) +static ssize_t __vtpm_proxy_read_unlocked(struct proxy_dev *proxy_dev, char __user *buf, + size_t count) { - struct proxy_dev *proxy_dev = filp->private_data; size_t len; - int sig, rc; - - sig = wait_event_interruptible(proxy_dev->wq, - proxy_dev->req_len != 0 || - !(proxy_dev->state & STATE_OPENED_FLAG)); - if (sig) - return -EINTR; - - mutex_lock(&proxy_dev->buf_lock); + int rc; - if (!(proxy_dev->state & STATE_OPENED_FLAG)) { - mutex_unlock(&proxy_dev->buf_lock); + if (!(proxy_dev->state & STATE_OPENED_FLAG)) return -EPIPE; - } len = proxy_dev->req_len; if (count < len || len > sizeof(proxy_dev->buffer)) { - mutex_unlock(&proxy_dev->buf_lock); pr_debug("Invalid size in recv: count=%zd, req_len=%zd\n", count, len); return -EIO; } - rc = copy_to_user(buf, proxy_dev->buffer, len); + if (buf) + rc = copy_to_user(buf, proxy_dev->buffer, len); + memset(proxy_dev->buffer, 0, len); proxy_dev->req_len = 0; if (!rc) proxy_dev->state |= STATE_WAIT_RESPONSE_FLAG; - mutex_unlock(&proxy_dev->buf_lock); - if (rc) return -EFAULT; return len; } -/** - * vtpm_proxy_fops_write - Write TPM responses on 'server side' - * - * @filp: file pointer - * @buf: write buffer - * @count: number of bytes to write - * @off: offset - * - * Return: - * Number of bytes read or negative error value - */ -static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf, - size_t count, loff_t *off) +static ssize_t __vtpm_proxy_write_unlocked(struct proxy_dev *proxy_dev, const char __user *buf, + size_t count) { - struct proxy_dev *proxy_dev = filp->private_data; - - mutex_lock(&proxy_dev->buf_lock); - - if (!(proxy_dev->state & STATE_OPENED_FLAG)) { - mutex_unlock(&proxy_dev->buf_lock); + if (!(proxy_dev->state & STATE_OPENED_FLAG)) return -EPIPE; - } if (count > sizeof(proxy_dev->buffer) || - !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) { - mutex_unlock(&proxy_dev->buf_lock); + !(proxy_dev->state & STATE_WAIT_RESPONSE_FLAG)) return -EIO; - } proxy_dev->state &= ~STATE_WAIT_RESPONSE_FLAG; proxy_dev->req_len = 0; - if (copy_from_user(proxy_dev->buffer, buf, count)) { - mutex_unlock(&proxy_dev->buf_lock); + if (buf && copy_from_user(proxy_dev->buffer, buf, count)) return -EFAULT; - } proxy_dev->resp_len = count; + return count; +} +static ssize_t __vtpm_proxy_read_write_unlocked(struct proxy_dev *proxy_dev, char __user *buf, + size_t count) +{ + ssize_t rc; + + do { + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count); + if (rc < 0) + break; + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, rc); + } while (0); + + return rc; +} + +/* + * See struct file_operations. + */ +static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf, + size_t count, loff_t *off) +{ + struct proxy_dev *proxy_dev = filp->private_data; + ssize_t rc; + int sig; + + sig = wait_event_interruptible(proxy_dev->wq, + proxy_dev->req_len != 0 || + !(proxy_dev->state & STATE_OPENED_FLAG)); + if (sig) + return -EINTR; + + mutex_lock(&proxy_dev->buf_lock); + rc = __vtpm_proxy_read_unlocked(proxy_dev, buf, count); mutex_unlock(&proxy_dev->buf_lock); + return rc; +} + +/* + * See struct file_operations. + */ +static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf, + size_t count, loff_t *off) +{ + struct proxy_dev *proxy_dev = filp->private_data; + int rc; + + mutex_lock(&proxy_dev->buf_lock); + rc = __vtpm_proxy_write_unlocked(proxy_dev, buf, count); + mutex_unlock(&proxy_dev->buf_lock); wake_up_interruptible(&proxy_dev->wq); - return count; + return rc; } /* @@ -295,28 +300,6 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) return len; } -static int vtpm_proxy_is_driver_command(struct tpm_chip *chip, - u8 *buf, size_t count) -{ - struct tpm_header *hdr = (struct tpm_header *)buf; - - if (count < sizeof(struct tpm_header)) - return 0; - - if (chip->flags & TPM_CHIP_FLAG_TPM2) { - switch (be32_to_cpu(hdr->ordinal)) { - case TPM2_CC_SET_LOCALITY: - return 1; - } - } else { - switch (be32_to_cpu(hdr->ordinal)) { - case TPM_ORD_SET_LOCALITY: - return 1; - } - } - return 0; -} - /* * Called when core TPM driver forwards TPM requests to 'server side'. * @@ -330,6 +313,7 @@ static int vtpm_proxy_is_driver_command(struct tpm_chip *chip, static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) { struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev); + unsigned int ord = ((struct tpm_header *)buf)->ordinal; if (count > sizeof(proxy_dev->buffer)) { dev_err(&chip->dev, @@ -338,9 +322,11 @@ static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count) return -EIO; } - if (!(proxy_dev->state & STATE_DRIVER_COMMAND) && - vtpm_proxy_is_driver_command(chip, buf, count)) - return -EFAULT; + if ((chip->flags & TPM_CHIP_FLAG_TPM2) && ord == TPM2_CC_SET_LOCALITY) + return -EINVAL; + + if (ord == TPM_ORD_SET_LOCALITY) + return -EINVAL; mutex_lock(&proxy_dev->buf_lock); @@ -409,12 +395,10 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality) return rc; tpm_buf_append_u8(&buf, locality); - proxy_dev->state |= STATE_DRIVER_COMMAND; - - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality"); - - proxy_dev->state &= ~STATE_DRIVER_COMMAND; - + mutex_lock(&proxy_dev->buf_lock); + memcpy(proxy_dev->buffer, buf.data, tpm_buf_length(&buf)); + rc = __vtpm_proxy_read_write_unlocked(proxy_dev, NULL, tpm_buf_length(&buf)); + mutex_unlock(&proxy_dev->buf_lock); if (rc < 0) { locality = rc; goto out;