MIME-Version: 1.0
References: <20211203093000.3714620-1-keescook@chromium.org>
 <CANpmjNPCjXp_0iQjdznpvS2vF-VvDkxJrtWnBAoj6TAU488VAQ@mail.gmail.com> <CE7F1A95-67C7-4BDA-B803-D93901EC2378@chromium.org>
In-Reply-To: <CE7F1A95-67C7-4BDA-B803-D93901EC2378@chromium.org>
From:   Marco Elver <elver@google.com>
Date:   Fri, 3 Dec 2021 17:53:15 +0100
Message-ID: <CANpmjNMCrLU0J3W+-=wihEaHXMytSfNy=iu-8-oO8JDvEUYAJw@mail.gmail.com>
Subject: Re: [PATCH] lib/test_ubsan: Silence compile-time array bounds warnings
To:     Kees Cook <keescook@chromium.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Jiri Olsa <jolsa@kernel.org>, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 3 Dec 2021 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> On December 3, 2021 2:49:53 AM PST, Marco Elver <elver@google.com> wrote:
[...]
> >Are there other warnings or only the one for the fsanitize=3Dobject-size
> >test? I think this is fine if there are other warnings.
>
> I will double check, but I think it's only the object-size test, which se=
ems to confirm my suspicion that -Warray-bounds provides sufficient coverag=
e and object-size can be removed.
>
> I have another patch I intend to send today for the sk_buff/sk_buff_head =
issue, as -Warray-bounds warns for that as well.

Nice.

Do you want to send the patch removing UBSAN_OBJECT_SIZE, or shall I
do it? Perhaps it ties in better with the rest of your patches which I
have no state of.

> >But, if it's only about the fsanitize=3Dobject-size test, I'm going to
> >propose something more drastic. :-)
>
> Are there any cases where object-size does a run-time check that couldn't=
 be done at compile time? That's the only reason I could see to keep it at =
this point, as -Warray-bounds can do the compile time checks.

No, I don't think so.

I stared at the LLVM code several times now, because I still couldn't
quite believe it myself, but I think it really doesn't do any dynamic
checks. Hence, why below I say it should have been a compiler warning.
As mentioned in the bugzilla bug, there's a FIXME in the LLVM code to
do a dynamic check with the help of fsanitize=3Daddress, but that never
happened. And that doesn't make much sense anyway if fsanitize=3Daddress
(viz. KASAN for us) is already on and does checking itself.

> >I had wanted to wait a bit and dig a little deeper, but I just posted
> >part of my analysis here:
> >https://bugzilla.kernel.org/show_bug.cgi?id=3D214861#c4
>
> Thanks, I'll refer to that in my sk_buff patch. It seems -Warray-bounds s=
uffers from the same conservativism about object casts, which is frustratin=
g on the one hand since the warning can be a false positive (cast vs access=
), but on the other, it does call attention to fragile arrangements which m=
aybe could do with adjustment.
>
> >My proposal is to remove UBSAN_OBJECT_SIZE and its related tests. The
> >bugzilla bug goes into the details, but the TLDR is:
> >1. fsanitize=3Dobject-size is incomplete,
> >2. it should have been a compiler warning,
> >3. for everything else there is KASAN which detects real OOB,
> >4. for GCC we already disable UBSAN_OBJECT_SIZE.
>
> And maybe:
> 5. -Warray-bounds provides the same coverage and is about to be enabled g=
lobally.

Yup, in which case the compiler warning already exists and point #2
above is moot.

Thanks,
-- Marco