Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190215235901.23541-1-baloo@gandi.net> <CAG48ez2tYizTKncevLF=AMQ2nm3D=SqGHH5bM5f-U0fhQ1nL9Q@mail.gmail.com>
 <alpine.DEB.2.21.1902161358160.1683@nanos.tec.linutronix.de>
 <4F2693EA-1553-4F09-9475-781305540DBC@amacapital.net> <20190216234702.GP2217@ZenIV.linux.org.uk>
 <20190217034121.bs3q3sgevexmdt3d@khany> <20190217042201.GU2217@ZenIV.linux.org.uk>
 <alpine.DEB.2.21.1902181347500.1549@nanos.tec.linutronix.de>
In-Reply-To: <alpine.DEB.2.21.1902181347500.1549@nanos.tec.linutronix.de>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Mon, 18 Feb 2019 11:15:44 -0800
Message-ID: <CALCETrXyard2OXmOafiLks3YuyO=ObbjDXB6NJo_08rL4M6azw@mail.gmail.com>
Subject: Re: [PATCH] x86: uaccess: fix regression in unsafe_get_user
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        Arthur Gautier <baloo@gandi.net>, Jann Horn <jannh@google.com>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        kernel list <linux-kernel@vger.kernel.org>,
        Pascal Bouchareine <pascal@gandi.net>
Content-Type: multipart/mixed; boundary="0000000000005fda6b05822ff4e5"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--0000000000005fda6b05822ff4e5
Content-Type: text/plain; charset="UTF-8"

On Mon, Feb 18, 2019 at 5:04 AM Thomas Gleixner <tglx@linutronix.de> wrote:

> > Another would be to have the buffer passed to flush_buffer() (i.e.
> > the callback of decompress_fn) allocated with 4 bytes of padding
> > past the part where the unpacked piece of data is placed for the
> > callback to find.  As in,
> >
> > diff --git a/lib/decompress_inflate.c b/lib/decompress_inflate.c
> > index 63b4b7eee138..ca3f7ecc9b35 100644
> > --- a/lib/decompress_inflate.c
> > +++ b/lib/decompress_inflate.c
> > @@ -48,7 +48,7 @@ STATIC int INIT __gunzip(unsigned char *buf, long len,
> >       rc = -1;
> >       if (flush) {
> >               out_len = 0x8000; /* 32 K */
> > -             out_buf = malloc(out_len);
> > +             out_buf = malloc(out_len + 4);
>
>   +8 actually.
>
> >       } else {
> >               if (!out_len)
> >                       out_len = ((size_t)~0) - (size_t)out_buf; /* no limit */
> >
> > for gunzip/decompress and similar ones for bzip2, etc.  The contents
> > layout doesn't have anything to do with that...
>
> Right. That works nicely.
>

This seems like it's just papering over the underlying problem: with
Jann's new checks in place, strncpy_from_user() is simply buggy.  Does
the patch below look decent?  It's only compile-tested, but it's
conceptually straightforward.  I was hoping I could get rid of the
check-maximum-address stuff, but it's needed for architectures where
the user range is adjacent to the kernel range (i.e. not x86_64).

Jann, I'm still unhappy that this code will write up to sizeof(long)-1
user-controlled garbage bytes in-bounds past the null-terminator in
the kernel buffer.  Do you think that's worth changing, too?  I don't
think it's a bug per se, but it seems like a nifty little wart for an
attacker to try to abuse.

On brief inspection, strnlen_user() does not have an equivalent bug.

--0000000000005fda6b05822ff4e5
Content-Type: text/x-patch; charset="US-ASCII"; name="strncpy.patch"
Content-Disposition: attachment; filename="strncpy.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_jsaps62u0>
X-Attachment-Id: f_jsaps62u0

ZGlmZiAtLWdpdCBhL2xpYi9zdHJuY3B5X2Zyb21fdXNlci5jIGIvbGliL3N0cm5jcHlfZnJvbV91
c2VyLmMKaW5kZXggNThlYWNkNDE1MjZjLi43MDlkNmVmZTBkNDIgMTAwNjQ0Ci0tLSBhL2xpYi9z
dHJuY3B5X2Zyb21fdXNlci5jCisrKyBiL2xpYi9zdHJuY3B5X2Zyb21fdXNlci5jCkBAIC0xMCwx
MiArMTAsNyBAQAogI2luY2x1ZGUgPGFzbS9ieXRlb3JkZXIuaD4KICNpbmNsdWRlIDxhc20vd29y
ZC1hdC1hLXRpbWUuaD4KIAotI2lmZGVmIENPTkZJR19IQVZFX0VGRklDSUVOVF9VTkFMSUdORURf
QUNDRVNTCi0jZGVmaW5lIElTX1VOQUxJR05FRChzcmMsIGRzdCkJMAotI2Vsc2UKLSNkZWZpbmUg
SVNfVU5BTElHTkVEKHNyYywgZHN0KQlcCi0JKCgobG9uZykgZHN0IHwgKGxvbmcpIHNyYykgJiAo
c2l6ZW9mKGxvbmcpIC0gMSkpCi0jZW5kaWYKKyNkZWZpbmUgSVNfVU5BTElHTkVEKGFkZHIpICgo
KGxvbmcpKGFkZHIpKSAmIChzaXplb2YobG9uZykgLSAxKSkKIAogLyoKICAqIERvIGEgc3RybmNw
eSwgcmV0dXJuIGxlbmd0aCBvZiBzdHJpbmcgd2l0aG91dCBmaW5hbCAnXDAnLgpAQCAtMzUsMTQg
KzMwLDM5IEBAIHN0YXRpYyBpbmxpbmUgbG9uZyBkb19zdHJuY3B5X2Zyb21fdXNlcihjaGFyICpk
c3QsIGNvbnN0IGNoYXIgX191c2VyICpzcmMsIGxvbmcKIAlpZiAobWF4ID4gY291bnQpCiAJCW1h
eCA9IGNvdW50OwogCi0JaWYgKElTX1VOQUxJR05FRChzcmMsIGRzdCkpCisJLyoKKwkgKiBGaXJz
dCBoYW5kbGUgYW55IHVuYWxpZ25lZCBwcmVmaXggb2Ygc3JjLgorCSAqLworCXdoaWxlIChtYXgg
JiYgSVNfVU5BTElHTkVEKHNyYytyZXMpKSB7CisJCWNoYXIgYzsKKworCQl1bnNhZmVfZ2V0X3Vz
ZXIoYywgc3JjK3JlcywgZWZhdWx0KTsKKwkJZHN0W3Jlc10gPSBjOworCQlpZiAoIWMpCisJCQly
ZXR1cm4gcmVzOworCQlyZXMrKzsKKwkJbWF4LS07CisJfQorCisJLyoKKwkgKiBOb3cgd2Uga25v
dyB0aGF0IHNyYyArIHJlcyBpcyBhbGlnbmVkLiAgSWYgZHN0IGlzIHVuYWxpZ25lZCBhbmQKKwkg
KiB3ZSBkb24ndCBoYXZlIGVmZmljaWVudCB1bmFsaWduZWQgYWNjZXNzLCB0aGVuIGtlZXAgZ29p
bmcgb25lCisJICogYnl0ZSBhdCBhIHRpbWUuICAoVGhpcyBjb3VsZCBiZSBvcHRpbWl6ZWQsIGJ1
dCBpdCB3b3VsZCBtYWtlCisJICogdGhlIGNvZGUgbW9yZSBjb21wbGljYXRlZC4KKwkgKi8KKyNp
Zm5kZWYgQ09ORklHX0hBVkVfRUZGSUNJRU5UX1VOQUxJR05FRF9BQ0NFU1MKKwlpZiAoSVNfVU5B
TElHTkVEKGRzdCArIHJlcykpCiAJCWdvdG8gYnl0ZV9hdF9hX3RpbWU7CisjZW5kaWYKIAogCXdo
aWxlIChtYXggPj0gc2l6ZW9mKHVuc2lnbmVkIGxvbmcpKSB7CisJCS8qCisJCSAqIHNyYyArIHJl
cyBpcyBhbGlnbmVkLCBzbyB0aGUgcmVhZHMgaW4gdGhpcyBsb29wIHdpbGwKKwkJICogbm90IGNy
b3NzIGEgcGFnZSBib3VuZGFyeS4KKwkJICovCiAJCXVuc2lnbmVkIGxvbmcgYywgZGF0YTsKIAot
CQkvKiBGYWxsIGJhY2sgdG8gYnl0ZS1hdC1hLXRpbWUgaWYgd2UgZ2V0IGEgcGFnZSBmYXVsdCAq
LwotCQl1bnNhZmVfZ2V0X3VzZXIoYywgKHVuc2lnbmVkIGxvbmcgX191c2VyICopKHNyYytyZXMp
LCBieXRlX2F0X2FfdGltZSk7CisJCXVuc2FmZV9nZXRfdXNlcihjLCAodW5zaWduZWQgbG9uZyBf
X3VzZXIgKikoc3JjK3JlcyksIGVmYXVsdCk7CiAKIAkJKih1bnNpZ25lZCBsb25nICopKGRzdCty
ZXMpID0gYzsKIAkJaWYgKGhhc196ZXJvKGMsICZkYXRhLCAmY29uc3RhbnRzKSkgewpAQCAtNTQs
NyArNzQsOSBAQCBzdGF0aWMgaW5saW5lIGxvbmcgZG9fc3RybmNweV9mcm9tX3VzZXIoY2hhciAq
ZHN0LCBjb25zdCBjaGFyIF9fdXNlciAqc3JjLCBsb25nCiAJCW1heCAtPSBzaXplb2YodW5zaWdu
ZWQgbG9uZyk7CiAJfQogCi1ieXRlX2F0X2FfdGltZToKKwkvKgorCSAqIEZpbmlzaCB0aGUgam9i
IG9uZSBieXRlIGF0IGEgdGltZS4KKwkgKi8KIAl3aGlsZSAobWF4KSB7CiAJCWNoYXIgYzsKIAo=
--0000000000005fda6b05822ff4e5--