[Xen-devel] Re: [PATCH 5/6] trace: fix security issues

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 01.07.10 at 10:32, George Dunlap <dunlapg@xxxxxxxxx> wrote:
> On Thu, Jul 1, 2010 at 9:04 AM, Jan Beulich <JBeulich@xxxxxxxxxx> wrote:
>> The point is to prevent the compiler from doing the memory reads
>> more than once (which it is permitted to do on non-volatiles). As
>> said in the submission comment, this could be done via barriers too,
>> but one of the two has to be there.
> 
> I understand what the volatile modifier is supposed to do. :-)
> (Although I think you've got it backwards -- it forces the compiler to
> do memory reads every time, instead of doing them only once and
> caching them in a register.)  My question is, why do you think the

I don't think so - I made each function read the fields exactly once,
storing into local variables as necessary.

> volatile modifier is necessary?  What kinds of situations are you
> trying to protect against?  What terrible havoc can a broken / rogue
> xentrace binary wreak upon the hypervisor if we don't have the
> volatiles in?

The issue we had got reported: A crashed hypervisor. The thing is
that code that uses fields that code outside the hypervisor may
modify must in no case imply that consistency checks done earlier
in the code still apply when re-reading the data from memory.
Hence we have to make sure that reads happen exactly once,
and checks plus any consumption happen on/from the values kept
locally, not by re-reading the shared buffer fields.

> Moreover, the purpose of volatile is slightly different than memory
> barriers.  AFAICT from the rather sparse documentation, "volatile"
> doesn't prevent a compiler from re-ordering memory accesses that it
> deems independent.  That's what the memory barriers are for.

Ordering isn't a problem here, but enforcing the read-once
requirement can be done either way afaict.

> At least one specific example where volatile helps would be appreciated.

static inline struct t_rec *next_record(const struct t_buf *buf)
{
    u32 x = buf->prod;
*** no read at all ***

    if ( !tb_init_done || bogus(x, buf->cons) )
*** read buf->prod ***
        return NULL;

    if ( x >= data_size )
*** read buf->prod again ***
        x -= data_size;
*** x = buf->prod - data_size; (reading buf->prod a third time) ***
*** else ***
*** x = buf->prod; ***

    ASSERT(x < data_size);

    return (struct t_rec *)&this_cpu(t_data)[x];
}

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel