[v2] tpm: vtpm_proxy: Double-check to avoid buffer overflow

Commit Message

Kees Cook Jan. 18, 2022, 6:36 p.m. UTC

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 <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Jason Gunthorpe <[e-mail]>
Cc: [e-mail]
Reviewed-by: Stefan Berger <[e-mail]>
Link: https://lore.kernel.org/lkml/[e-mail]
Signed-off-by: Kees Cook <[e-mail]>
---
v1: https://lore.kernel.org/lkml/[e-mail]
v2: make commit log more accurate, add Reviewed-by
---
 drivers/char/tpm/tpm_vtpm_proxy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jann Horn Jan. 18, 2022, 7:32 p.m. UTC | #1

On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[e-mail]> 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?

Jason Gunthorpe Jan. 18, 2022, 7:39 p.m. UTC | #2

On Tue, Jan 18, 2022 at 08:32:43PM +0100, Jann Horn wrote:
> On Tue, Jan 18, 2022 at 7:37 PM Kees Cook <[e-mail]> 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

Kees Cook Jan. 18, 2022, 9:20 p.m. UTC | #3

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 <[e-mail]> 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.

Jason Gunthorpe Jan. 18, 2022, 11:33 p.m. UTC | #4

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

Jann Horn Jan. 19, 2022, 12:42 a.m. UTC | #5

On Tue, Jan 18, 2022 at 10:20 PM Kees Cook <[e-mail]> 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 <[e-mail]> 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...)

Patch

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);