Date:   Wed, 23 Feb 2022 11:06:03 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jakob <jakobkoschel@...il.com>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list
 iterator after the loop

On Wed, Feb 23, 2022 at 6:18 AM Jakob <jakobkoschel@...il.com> wrote:
>
> However, in this example, 'tmp' will be a out-of-bounds pointer
> if the loop did finish without hitting the break, so the check past the
> loop *could* match 'mdev' even though no break was ever met.

So just as context for others, since I was hit with the same confusion
and didn't see what the relevance was for type speculation, when these
patches seemed to be not about speculation at all.

The background for this is that the list_for_each_entry() will set the
iterator variable (here 'tmp') to be not the actual internal list
pointer, but the pointer to the containing type (which is the whole
'entry' part of the name, of course).

And that is all good and true, but it's true only *WITHIN* that loop.
At the exit condition, it will have hit the 'head' of the list, and
the type that contains the list head is *not* necessarily the same
type as the list entries.

So that's where the type confusion comes from: if you access the list
iterator outside the loop, and it could have fallen off the end of the
loop, the list iterator pointer is now not actually really a valid
pointer of that 'entry' type at all.

And as such, you not only can't dereference it, but you also shouldn't
even compare pointer values - because the pointer arithmetic that was
valid for loop entries is not valid for the HEAD entry that is
embedded in another type. So the pointer arithmetic might have turned
it into a pointer outside the real container of the HEAD, and might
actually match something else.

Now, there are a number of reasons I really dislike the current RFC
patch series, so I'm not claiming the patch is something we should
apply as-is, but I'm trying to clarify why Jakob is doing what he's
doing (because clearly I wasn't the only one taken  by surprise by
it).

The reasons I don't like it is:

 - patches like these are very random. And very hard to read (and very
easy to re-introduce the bug).

 - I think it conflates the non-speculative "use pointer of the wrong
type" issue like in this patch with the speculation

 - I'm not even convinced that 'list_for_each_entry()' is that special
wrt speculative type accesses, considering that we have other uses of
doubly linked list *everywhere* - and it can happen in a lot of other
situations anyway, so it all seems to be a bit ad hoc.

but I do think the problem is real.

So elsewhere I suggested that the fix to "you can't use the pointer
outside the loop" be made to literally disallow it (using C99 for-loop
variables seems the cleanest model), and have the compiler refuse to
touch code that tries to use the loop iterator outside.

And that is then entirely separate from the issue of actual
speculative accesses (but honestly, I think that's a "you have to
teach the compiler not to do them" issue, not a "let's randomly change
*one* of our loop walkers).

                Linus

Powered by blists - more mailing lists