lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 18 Jan 2022 13:20:40 -0800
From:   Kees Cook <>
To:     Jason Gunthorpe <>
Cc:     Jann Horn <>, Peter Huewe <>,
        Jarkko Sakkinen <>,,
        Stefan Berger <>,,
Subject: Re: [PATCH v2] tpm: vtpm_proxy: Double-check to avoid buffer overflow

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

Powered by blists - more mailing lists