Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201012110557.GK25311@zn.tnic>
In-Reply-To: <20201012110557.GK25311@zn.tnic>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 12 Oct 2020 11:11:35 -0700
Message-ID: <CAHk-=wiVoLoZS1v9SCQzH4mkpz6akE3pPrHPxM2hamOXPcaW9w@mail.gmail.com>
Subject: Re: [GIT PULL] x86/asm updates for v5.10
To:     Borislav Petkov <bp@suse.de>, Uros Bizjak <ubizjak@gmail.com>
Cc:     x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 12, 2020 at 4:06 AM Borislav Petkov <bp@suse.de> wrote:
>
> * Use XORL instead of XORQ to avoid a REX prefix and save some bytes in
> the .fixup section, by Uros Bizjak.

I think this one is actually buggy.

For the 1-byte case, it does this:

     __get_user_asm(x_u8__, ptr, retval, "b", "=q");

and ends up doing "xorl" on a register that we told the compiler is a
byte register (with that "=q")

Yes, it uses "%k[output]" to turn that byte register into the word
version of the register, but there's no fundamental reason why the
register might not be something like "%ah".

Does the "xorl" work? Does it build? Yes, and yes.

But maybe %al contains SOMETHING ELSE, and it now clears that too,
because the asm is basically doing something completely different than
what we told the compiler it would do.

Now, afaik, gcc (and presumably clang) basically almost never use the
high byte registers. But I still think this patch is fundamentally
wrong and conceptually completely buggy, even if it might work in
practice.

Also, I'm going to uninline this nasty __get_user() function anyway
for 5.10, so the patch ends up being not just wrong, but pointless.
This is not some kind of hot code that should be optimized, and the
extra byte is not a lot to worry about.

Annoying. Because the other patch in this pull request is fine, and
people want it.

But I'm going to skip this pull request, because I really think it's
dangerously and subtly buggy even if there might not be any case that
matters in reality.

                   Linus