linux-kernel - Re: [patch V2 08/16] x86/ioperm: Add bitmap sequence number

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1911121809020.1833@nanos.tec.linutronix.de>
Date:   Tue, 12 Nov 2019 18:10:56 +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>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [patch V2 08/16] x86/ioperm: Add bitmap sequence number

On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> >         /*
> > +        * The bitmap pointer and the sequence number of the last active
> > +        * bitmap. last_bitmap cannot be dereferenced. It's solely for
> > +        * comparison.
> > +        */
> > +       struct io_bitmap        *last_bitmap;
> > +       u64                     last_sequence;
> > +
> > +       /*
> >          * Store the dirty size of the last io bitmap offender. The next
> >          * one will have to do the cleanup as the switch out to a non io
> >          * bitmap user will just set x86_tss.io_bitmap_base to a value
> 
> Why is all this stuff in the TSS?  Would it make more sense in a
> percpu variable tss_state?  By putting it in the TSS, you're putting
> it in cpu_entry_area, which is at least a bit odd if not an actual
> security problem.
> 
> And why do you need a last_bitmap pointer?  I thin that comparing just
> last_sequence should be enough.  Keeping last_bitmap around at all is
> asking for trouble, since it might point to freed memory.

The bitmap pointer is pointless as I said in an earlier reply to Peter. It
will go away. The sequence number and the dirty size are surely not a
problem leakage wise, but yes, we could put it into a per cpu variable as
well. Not sure whether it buys much.
 
> > -               memcpy(tss->io_bitmap_bytes, iobm->bitmap_bytes,
> > -                      max(tss->io_bitmap_prev_max, iobm->io_bitmap_max));
> > +               if (tss->last_bitmap != iobm ||
> > +                   tss->last_sequence != iobm->sequence)
> 
> As above, I think this could just be if (tss->last_sequence !=
> iobm->sequence) or even if (this_cpu_read(cpu_tss_state.iobm_sequence)
> != iobm->sequence).

Already fixed as per Peter's comments.

Thanks,

	tglx