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

[Kees Cook <keescook@chromium.org>]

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 <keescook@chromium.org> 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