linux-kernel - Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

lists.openwall.net		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 linux-cve-announce 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]

Message-ID: <CAHk-=wg1RdFQ6OGb_H4ZJoUwEr-gk11QXeQx63n91m0tvVUdZw@mail.gmail.com>
Date:   Thu, 17 Feb 2022 11:28:58 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jakob Koschel <jakobkoschel@...il.com>
Cc:     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 03/13] usb: remove the usage of the list iterator
 after the loop

On Thu, Feb 17, 2022 at 10:50 AM Jakob Koschel <jakobkoschel@...il.com> wrote:
>
> It is unsafe to assume that &req->req != _req can only evaluate
> to false if the break within the list iterator is hit.

I don't understand what problem you are trying to fix.

Since "req" absolutely *has* to be stable for any of this code to be
valid, then "&req->req" is stable and unambiguous too. The *only* way
_req can point to it would be if we finished the iteration properly.

So I don't see the unsafeness.

Note that all this work with "speculative" execution fundamentally CAN
NOT affect semantics of the code, yet this patch makes statements
about exactly that.

That's not how CPU speculation works.

CPU speculation can expose hidden information that is not
"semantically important" (typically through cache access patterns, but
that's not the only way). So it might be exposing information it
shouldn't.

But if speculation is actually changing semantics, then it's no longer
"speculation" - it's just a bug, plain and simple (either a software
bug due to insufficient serialization, or an actual hardware bug).

IOW, I don't want to see these kinds of apparently pointless changes
to list walking. The patches should explain what that SECONDARY hidden
value you try to protect actually is for each case.

This patch is basically not sensible. It just moves code around in a
way that the compiler could have done anyway (or the compiler could
decide to undo). It doesn't explain what the magic protected value is
that shouldn't be leaked, and it leaves the code just looking odd and
pointless.

                   Linus