* [PATCH] tpm: vtpm_proxy: Avoid device-originated buffer overflow
@ 2022-01-13 0:27 Kees Cook
2022-01-13 21:19 ` Stefan Berger
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2022-01-13 0:27 UTC (permalink / raw)
To: Peter Huewe
Cc: Kees Cook, Jarkko Sakkinen, Jason Gunthorpe, linux-integrity,
linux-kernel, linux-hardening
When building with -Warray-bounds, this warning was emitted:
In function 'memset',
inlined from 'vtpm_proxy_fops_read' at drivers/char/tpm/tpm_vtpm_proxy.c:102:2:
./include/linux/fortify-string.h:43:33: warning: '__builtin_memset' pointer overflow between offset 164 and size [2147483648, 4294967295]
[-Warray-bounds]
43 | #define __underlying_memset __builtin_memset
| ^
There was no checking of the req_len value from the device. A malicious
(or buggy) device could end up leaking (and when wiping) memory contents
beyond the end of the proxy buffer.
Cc: Peter Huewe <peterhuewe@gmx.de>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-integrity@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/char/tpm/tpm_vtpm_proxy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 91c772e38bb5..5c865987ba5c 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -91,7 +91,7 @@ static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
len = proxy_dev->req_len;
- if (count < 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);
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tpm: vtpm_proxy: Avoid device-originated buffer overflow
2022-01-13 0:27 [PATCH] tpm: vtpm_proxy: Avoid device-originated buffer overflow Kees Cook
@ 2022-01-13 21:19 ` Stefan Berger
2022-01-18 18:35 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Berger @ 2022-01-13 21:19 UTC (permalink / raw)
To: Kees Cook, Peter Huewe
Cc: Jarkko Sakkinen, Jason Gunthorpe, linux-integrity, linux-kernel,
linux-hardening
On 1/12/22 19:27, Kees Cook wrote:
> When building with -Warray-bounds, this warning was emitted:
>
> In function 'memset',
> inlined from 'vtpm_proxy_fops_read' at drivers/char/tpm/tpm_vtpm_proxy.c:102:2:
> ./include/linux/fortify-string.h:43:33: warning: '__builtin_memset' pointer overflow between offset 164 and size [2147483648, 4294967295]
> [-Warray-bounds]
> 43 | #define __underlying_memset __builtin_memset
> | ^
>
> There was no checking of the req_len value from the device. A malicious
> (or buggy) device could end up leaking (and when wiping) memory contents
> beyond the end of the proxy buffer.
>
> Cc: Peter Huewe <peterhuewe@gmx.de>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: linux-integrity@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/char/tpm/tpm_vtpm_proxy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 91c772e38bb5..5c865987ba5c 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -91,7 +91,7 @@ static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
>
> len = proxy_dev->req_len;
>
> - if (count < 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);
Thanks for this patch.
I just want to clarify this. In vtpm_proxy_tpm_op_send() we have the
only place that sets req_len to a value larger than 0:
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);
if (count > sizeof(proxy_dev->buffer)) {
dev_err(&chip->dev,
"Invalid size in send: count=%zd, buffer size=%zd\n",
count, sizeof(proxy_dev->buffer));
return -EIO;
}
[...]
proxy_dev->req_len = count;
memcpy(proxy_dev->buffer, buf, count);
[...]
}
The above makes sure that we cannot copy more bytes into the
proxy_dev->buffer than the what the buffer has bytes for.
It then sets req_len to a valid value that is less or equal to the
buffer size.
Considering this your check above seems to only be there to make the
compiler happy but otherwise I don't see that this is a real problem
with a buffer overflow?!
Nevertheless, let all those compilers be happy:
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tpm: vtpm_proxy: Avoid device-originated buffer overflow
2022-01-13 21:19 ` Stefan Berger
@ 2022-01-18 18:35 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2022-01-18 18:35 UTC (permalink / raw)
To: Stefan Berger
Cc: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, linux-integrity,
linux-kernel, linux-hardening
On Thu, Jan 13, 2022 at 04:19:32PM -0500, Stefan Berger wrote:
> I just want to clarify this. In vtpm_proxy_tpm_op_send() we have the only
> place that sets req_len to a value larger than 0:
>
> 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);
>
> if (count > sizeof(proxy_dev->buffer)) {
> dev_err(&chip->dev,
> "Invalid size in send: count=%zd, buffer size=%zd\n",
> count, sizeof(proxy_dev->buffer));
> return -EIO;
> }
>
> [...]
>
> proxy_dev->req_len = count;
> memcpy(proxy_dev->buffer, buf, count);
>
> [...]
>
> }
>
>
> The above makes sure that we cannot copy more bytes into the
> proxy_dev->buffer than the what the buffer has bytes for.
>
> It then sets req_len to a valid value that is less or equal to the buffer
> size.
>
> Considering this your check above seems to only be there to make the
> compiler happy but otherwise I don't see that this is a real problem with a
> buffer overflow?!
>
> Nevertheless, let all those compilers be happy:
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Ah yes, thanks! I'll reword the commit log for v2. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-18 18:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 0:27 [PATCH] tpm: vtpm_proxy: Avoid device-originated buffer overflow Kees Cook
2022-01-13 21:19 ` Stefan Berger
2022-01-18 18:35 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).