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

[Thomas Gleixner]

On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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