linux-kernel - Re: [patch V2 06/16] x86/io: Speedup schedule out of I/O bitmap user

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:08:27 +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 06/16] x86/io: Speedup schedule out of I/O bitmap
 user

On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> > @@ -50,6 +48,11 @@ long ksys_ioperm(unsigned long from, uns
> >                  * limit correct.
> >                  */
> >                 preempt_disable();
> > +               t->io_bitmap_ptr = bitmap;
> > +               set_thread_flag(TIF_IO_BITMAP);
> > +               /* Make the bitmap base in the TSS valid */
> > +               tss = this_cpu_ptr(&cpu_tss_rw);
> > +               tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> >                 refresh_tss_limit();
> >                 preempt_enable();
> >         }
> 
> It's not shown in the diff, but the very next line of code turns
> preemption back off.  This means that we might schedule right here
> with TIF_IO_BITMAP set, the base set to VALID, but the wrong data in
> the bitmap.  I *think* this will actually end up being okay, but it
> certainly makes understanding the code harder.  Can you adjust the
> code so that preemption stays off?
> 
> More importantly, the code below this modifies the TSS copy in place
> instead of writing a whole new copy.  But now that you've added your
> optimization, the TSS copy might be *someone else's* IO bitmap.  So I
> think you might end up with more io ports allowed than you intended.
> For example:
> 
> Task A uses ioperm() to enable all ports.
> Switch to task B.  Now the TSS base is INVALID but all bitmap bits are still 0.
> Task B calls ioperm().
> 
> The code will set the base to VALID and will correctly set up the
> thread's copy of the bitmap, but I think the copy will only update the
> bits 0 through whatever ioperm() touched and not the bits above that
> in the TSS.

Yeah, you are right. Did not think about that. Will fix that up.
 
> I would believe that this is fixed later in your patch set.  If so,
> perhaps you should just memcpy() the whole thing without trying to
> optimize in this patch and then let the changes later re-optimize it
> as appropriate.  IOW change memcpy(tss->io_bitmap, t->io_bitmap_ptr,
> bytes_updated); to memcpy(..., BYTES_PER_LONG * IO_BITMAP_LONGS) or
> similar.

Right.

Thanks for spotting that!

       tglx