linux-kernel - Re: [patch V2 09/16] x86/ioperm: Move TSS bitmap update to exit to user work

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]

Date:   Tue, 12 Nov 2019 18:20:14 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Andy Lutomirski <luto@...nel.org>
cc:     LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Willy Tarreau <w@....eu>, Juergen Gross <jgross@...e.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [patch V2 09/16] x86/ioperm: Move TSS bitmap update to exit to
 user work

On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> > There is no point to update the TSS bitmap for tasks which use I/O bitmaps
> > on every context switch. It's enough to update it right before exiting to
> > user space.
> +
> > +static inline void switch_to_bitmap(unsigned long tifp)
> > +{
> > +       /*
> > +        * Invalidate I/O bitmap if the previous task used it. If the next
> > +        * task has an I/O bitmap it will handle it on exit to user mode.
> > +        */
> > +       if (tifp & _TIF_IO_BITMAP)
> > +               tss_invalidate_io_bitmap(this_cpu_ptr(&cpu_tss_rw));
> > +}
> 
> Shouldn't you be invalidating the io bitmap if the *next* task doesn't
> use?  Or is the rule that, when a non-io-bitmap-using task is running,
> even in kernel mode, the io bitmap is always invalid.

Well it does not make much of a difference whether we do the above or
!(tifn & _TIF_IO_BITMAP). We always end up in that code when one of the
involved tasks has TIF_IO_BITMAP set. I decided to use the sched out check
because that makes it clear that this is the end of the valid I/O
bitmap. If the next task has TIF_IO_BITMAP set as well, then it will anyway
end up in the exit to user mode update code. Clearing it here ensures that
even if the exit to user mode malfunctions the bitmap cannot be leaked.

> As it stands, you need exit_thread() to invalidate the bitmap.  I
> assume it does, but I can't easily see it in the middle of the series
> like this.

It does.
 
> IOW your code might be fine, but it could at least use some comments
> in appropriate places (exit_to_usermode_loop()?) that we guarantee
> that, if the bit is *clear*, then the TSS has the io bitmap marked
> invalid.  And add an assertion under CONFIG_DEBUG_ENTRY.
> 
> Also, do you need to update EXIT_TO_USERMODE_LOOP_FLAGS?

No, the TIF_IO_BITMAP check is done once after the loop has run and it
would not make any sense in the loop as TIF_IO_BITMAP cannot be cleared
there and we'd loop forever. The other usermode loop flags are transient.

Thanks,

	tglx