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. To keep this code robust,
and to silence the compiler warning, check the size before attempting
a memset().
Cc: Peter Huewe <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: [email protected]
Reviewed-by: Stefan Berger <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
Signed-off-by: Kees Cook <[email protected]>
---
v1: https://lore.kernel.org/lkml/[email protected]
v2: make commit log more accurate, add Reviewed-by
---
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
On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[email protected]> 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
> | ^
Can you explain what that compiler warning actually means, and which
compiler it is from? Is this from a 32-bit or a 64-bit architecture?
It sounds like the compiler (GCC?) is hallucinating a codepath on
which "len" is guaranteed to be >=2147483648, right? Why is it doing
that? Is this some kinda side effect from the fortify code?
On Tue, Jan 18, 2022 at 08:32:43PM +0100, Jann Horn wrote:
> On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[email protected]> 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
> > | ^
>
> Can you explain what that compiler warning actually means, and which
> compiler it is from? Is this from a 32-bit or a 64-bit architecture?
>
> It sounds like the compiler (GCC?) is hallucinating a codepath on
> which "len" is guaranteed to be >=2147483648, right? Why is it doing
> that? Is this some kinda side effect from the fortify code?
I agree, this looks bogus, or at least the commit message neeeds alot
more explaining.
static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
if (count > sizeof(proxy_dev->buffer))
[...]
proxy_dev->req_len = count;
Not clear how req_len can be larger than sizeof(buffer)?
Jason
On Tue, Jan 18, 2022 at 03:39:31PM -0400, Jason Gunthorpe wrote:
> On Tue, Jan 18, 2022 at 08:32:43PM +0100, Jann Horn wrote:
> > On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[email protected]> 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
> > > | ^
> >
> > Can you explain what that compiler warning actually means, and which
> > compiler it is from? Is this from a 32-bit or a 64-bit architecture?
This is from ARCH=i386
> >
> > It sounds like the compiler (GCC?) is hallucinating a codepath on
Yes, GCC 11.2.
> > which "len" is guaranteed to be >=2147483648, right? Why is it doing
> > that? Is this some kinda side effect from the fortify code?
Right; I don't know what triggered it. I assume the "count" comparison.
The warning is generated with or without CONFIG_FORTIFY_SOURCE. It is
from adding -Warray-bounds. This is one of the last places in the kernel
where a warning is being thrown for this option, and it has found a lot
of real bugs, so Gustavo and I have been working to get the build
warning-clean so we can enable it globally.
> I agree, this looks bogus, or at least the commit message neeeds alot
> more explaining.
>
> static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
>
> if (count > sizeof(proxy_dev->buffer))
> [...]
> proxy_dev->req_len = count;
>
> Not clear how req_len can be larger than sizeof(buffer)?
Given the current code, I agree: it's not possible.
As for the cause of the warning, my assumption is that since the compiler
only has visibility into vtpm_proxy_fops_read(), and sees size_t len set
from ((struct proxy_dev *)filp->private_data)->req_len, and it performs
range checking perhaps triggered by the "count" comparison:
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;
size_t len;
...
len = proxy_dev->req_len;
if (count < len) {
...
return -EIO;
}
rc = copy_to_user(buf, proxy_dev->buffer, len);
memset(proxy_dev->buffer, 0, len);
I haven't been able to reproduce the specific cause of why GCC decided to
do the bounds checking, but it's not an unreasonable thing to check for,
just for robustness.
--
Kees Cook
On Tue, Jan 18, 2022 at 01:20:40PM -0800, Kees Cook wrote:
> I haven't been able to reproduce the specific cause of why GCC decided to
> do the bounds checking, but it's not an unreasonable thing to check for,
> just for robustness.
Well, the commit message should explain this is to silence a compiler
bug and maybe put some colour on what version(s) are actually buggy..
Jason
On Tue, Jan 18, 2022 at 10:20 PM Kees Cook <[email protected]> wrote:
> On Tue, Jan 18, 2022 at 03:39:31PM -0400, Jason Gunthorpe wrote:
> > On Tue, Jan 18, 2022 at 08:32:43PM +0100, Jann Horn wrote:
> > > On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[email protected]> 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
> > > > | ^
> > >
> > > Can you explain what that compiler warning actually means, and which
> > > compiler it is from? Is this from a 32-bit or a 64-bit architecture?
>
> This is from ARCH=i386
>
> > >
> > > It sounds like the compiler (GCC?) is hallucinating a codepath on
>
> Yes, GCC 11.2.
>
> > > which "len" is guaranteed to be >=2147483648, right? Why is it doing
> > > that? Is this some kinda side effect from the fortify code?
>
> Right; I don't know what triggered it. I assume the "count" comparison.
> The warning is generated with or without CONFIG_FORTIFY_SOURCE. It is
> from adding -Warray-bounds. This is one of the last places in the kernel
> where a warning is being thrown for this option, and it has found a lot
> of real bugs, so Gustavo and I have been working to get the build
> warning-clean so we can enable it globally.
>
> > I agree, this looks bogus, or at least the commit message neeeds alot
> > more explaining.
> >
> > static int vtpm_proxy_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
> >
> > if (count > sizeof(proxy_dev->buffer))
> > [...]
> > proxy_dev->req_len = count;
> >
> > Not clear how req_len can be larger than sizeof(buffer)?
>
> Given the current code, I agree: it's not possible.
>
> As for the cause of the warning, my assumption is that since the compiler
> only has visibility into vtpm_proxy_fops_read(), and sees size_t len set
> from ((struct proxy_dev *)filp->private_data)->req_len, and it performs
> range checking perhaps triggered by the "count" comparison:
>
>
> 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;
> size_t len;
> ...
> len = proxy_dev->req_len;
>
> if (count < len) {
> ...
> return -EIO;
> }
>
> rc = copy_to_user(buf, proxy_dev->buffer, len);
> memset(proxy_dev->buffer, 0, len);
>
>
> I haven't been able to reproduce the specific cause of why GCC decided to
> do the bounds checking, but it's not an unreasonable thing to check for,
> just for robustness.
Ok, I think this is what's happening:
$ cat bogus_bounds_warning_small.i
struct proxy_dev {
unsigned char buffer[4096];
};
long state;
void vtpm_proxy_fops_read(struct proxy_dev *proxy_dev, unsigned int len) {
/*
* sz == SIZE_MAX == -1 because the compiler can't prove whether proxy_dev
* points to an array or a single object and we're using the type-0 version.
*/
int sz = __builtin_object_size(proxy_dev->buffer, 0);
_Bool check_result;
/* always false but must keep this check to trigger the warning */
if (sz >= 0 && sz < len) {
check_result = 0;
/*
* compiler forks the rest of the function starting at this check, probably
* because it sees that a branch further down has a condition that depends on
* which branch we took here
*/
} else if (len > 0x7fffffff/*INT_MAX*/) {
check_result = 0;
} else {
check_result = 1;
}
/*
* this part is basically duplicated, it is compiled once for the
* len<=0x7fffffff case and once for the len>0x7fffffff case
*/
__builtin_memset(proxy_dev->buffer, 0, len);
if (check_result)
state |= 1;
}
$ gcc -ggdb -std=gnu89 -Warray-bounds -m32 -mregparm=3 -fno-pic
-march=i686 -O2 -c -o bogus_bounds_warning.o
bogus_bounds_warning_small.i
bogus_bounds_warning_small.i: In function ‘vtpm_proxy_fops_read’:
bogus_bounds_warning_small.i:32:3: warning: ‘__builtin_memset’
specified bound between 2147483648 and 4294967295 exceeds maximum
object size 2147483647 [-Wstringop-overflow=]
32 | __builtin_memset(proxy_dev->buffer, 0, len);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Here's what the CFG of the generated machine code looks like - you can
see how the function is split up starting at the "len > 0x7fffffff"
check: https://var.thejh.net/gcc_bounds_warning_cfg.png
(You can also see how the two copies of __builtin_memset() generate
some pretty gross and bloated code...)
