linux-kernel - Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

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: <alpine.DEB.2.21.1911071856420.27903@nanos.tec.linutronix.de>
Date:   Thu, 7 Nov 2019 19:02:27 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Ingo Molnar <mingo@...nel.org>
cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Stephen Hemminger <stephen@...workplumber.org>,
        Willy Tarreau <w@....eu>, Juergen Gross <jgross@...e.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage
 further

On Thu, 7 Nov 2019, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@...utronix.de> wrote:
> This might seem like a small detail, but since we do the range tracking 
> and copying at byte granularity anyway, why not do the zero range search 
> at byte granularity as well?
> 
> I bet it's faster and simpler as well than the bit-searching.

Not necessarily. The bitmap search uses unsigned long internally at least
to the point where it finds a zero bit.

But probably we should just ditch that optimization and rather have the
sequence number to figure out whether something needs to be copied at all.

> > +	if (first >= IO_BITMAP_BITS) {
> > +		/*
> > +		 * If the resulting bitmap has all permissions dropped, clear
> > +		 * TIF_IO_BITMAP and set the IO bitmap offset in the TSS to
> > +		 * invalid. Deallocate both the new and the thread's bitmap.
> > +		 */
> > +		clear_thread_flag(TIF_IO_BITMAP);
> > +		tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_INVALID;
> > +		tofree = bitmap;
> > +		bitmap = NULL;
> 
> BTW., wouldn't it be simpler to just say that if a thread uses IO ops 
> even once, it gets a bitmap and that's it? I.e. we could further simplify 
> this seldom used piece of code.

Maybe.
 
> > +	} else {
> >  		/*
> > +		 * I/O bitmap contains zero bits. Set TIF_IO_BITMAP, make
> > +		 * the bitmap offset valid and make sure that the TSS limit
> > +		 * is correct. It might have been wreckaged by a VMEXiT.
> >  		 */
> > +		set_thread_flag(TIF_IO_BITMAP);
> > +		tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> >  		refresh_tss_limit();
> >  	}
> 
> I'm wondering, shouldn't we call refresh_tss_limit() in both branches, or 
> is a VMEXIT-wreckaged TSS limit harmless if we otherwise have 
> io_bitmap_base set to IO_BITMAP_OFFSET_INVALID?

Yes. because the VMEXIT wreckage restricts TSS limit to 0x67 which is the
end of the hardware tss struct. As the invalid offset points in any case
outside the TSS limit it does not matter. It always #GP's when an I/O
access happens in user space.

Thanks,

	tglx