Re: [Xen-devel] [PATCH 1/7] introduce time managment in xtf

[Paul Semel <semelpaul@xxxxxxxxx>]

On 04/07/2018 10:39 PM, Andrew Cooper wrot>>>>> However, both of your 
patches have (different) barrier issues, and
> > > > > different (mis)uses of the shared memory clocks, which will need to be
> > > > > addressed. One general comment for the full series is to not bother
> > > > > trying to make time 32bits in a 32bit build.  XTF is an entirely
> > > > > self-contained binary, and I don't much care for legacy behaviours for
> > > > > legacy sake.
> > Alright, so I took a look at all of this. So, if I understood it well,
> > you want me to drop all the `uint32_t` part to only have 64 bits.
> > But are you sure that it works the same on 32bits architecture ?
> At the end of the day, all you are passing is units of nanoseconds of
> larger.  The parameter passing won't be identical between 32 or 64bit
> builds, but everything else will be fine.
>
> > Also, I have another question. I don't see any place where I have
> > memory barrier issues, but I am certainely missing something. Can you
> > elaborate on this one ?
> Your code does:
>
> > +    do
> > +    {
> > +        do
> > +        {
> > +            wc_version = shared_info.wc_version ;
> > +            version = shared_info.vcpu_info[0].time.version;
> > +        } while ( (version & 1) == 1 || (wc_version & 1) == 1);
> > +
> > +        system_time = shared_info.vcpu_info[0].time.system_time;
> > +        old_tsc = shared_info.vcpu_info[0].time.tsc_timestamp;
> > +    } while (
> > +            version != shared_info.vcpu_info[0].time.version ||
> > +            wc_version != shared_info.wc_version
> > +            );
> First of all, I'm not sure why you are looking at wc_version.  You are
> only reading data out of shared_info.vcpu_info[0].time so you should
> only need to check time.version
>
> That said, it is critical to force two things:
> 1) The first read of version is strictly before
> system_time/tsc_timestamp, and the second read is afterwards.
> 2) That the compiler can't optimise the code by making repeated reads of
> shared memory.
>
> 1 is a correctness issue and C, whose memory model is that there are no
> unknown updates to memory, sees that it hasn't written to time.version,
> and therefore can optimises the entire outer do loop to a single iteration.
>
> 2 is a security issue and we've had many XSAs in the past.  The worst
> example IIRC was a compiler which managed to use a jump table, using a
> 32bit integer index read from shared memory.  When using shared memory,
> it is critical to force the compiler to read a value and stash it
> locally (either in a live register, or on the stack), then audit its
> value, then act upon the value.
>
> This way, no optimisation the compiler can make code which will result
> in a repeated read of memory, where a malicious advisory could change
> the value between reads.
>
> 1 strictly needs memory barriers to make work, and in this case
> smp_rmb().  Things tend to work on x86 because of the fairly restricted
> memory model, but architectures such as ARM with weaker memory models
> typically tends to explode in funny ways.
>
> 2 could be fixed with ACCESS_ONCE() to explicitly tell the compiler that
> this is a volatile access and the value in memory may no longer be the same.
>
> A sample (which I think is correct, but you never quite know for sure
> with barriers!) is:
>
> >      do {
> >          uint32_t ver1, ver2;
> >
> >          do {
> >              ver1 = shared_info.vcpu_info[0].time.version;
> >              smp_rmb();
> >          } while ( (ver1 & 1) == 1 );
> >
> >          system_time = shared_info.vcpu_info[0].time.system_time;
> >          old_tsc = shared_info.vcpu_info[0].time.tsc_timestamp;
> >          smp_rmb();
> >          ver2 = shared_info.vcpu_info[0].time.version;
> >          smp_rmb();
> >
> >      } while ( ver1 != ver2 );
> The smp_rmb() are full CPU memory barriers (enforcing the ordering of
> reads even from remote cores), and compiler barriers (preventing the
> compiler from reodering memory accesses across the barrier).
>
> Therefore, ver1 and ver2 are forced to be kept in registers, or spilled
> onto the stack.
>
> ~Andrew
Thank you very much for replying ! I completely missed this function, I 
do really apologize about it !
Effectively, there is serious issues here. As far as I can remember, I 
inspired myself by another OS code to write this function (as I didn't 
know really much how the time thing was working in Xen).
Thanks for writing this piece of code, I think I will use it and add 
some ACCESS_ONCE on my reads of the different variables in the shared 
memory (version, system_time etc..).
Anyway, I already got rid of the 32 bits part, so that even 32 bits 
architectures are using 64 bits integer (by making sure to use the 
`divmod64` function 🙂) for the division and modulos.
I will change this part of the code this night or maybe tomorrow and 
send you another version of the patch !
--
Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel