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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 30.06.10 at 18:58, George Dunlap <dunlapg@xxxxxxxxx> wrote:
> Most of the patch, other than the volatiles, looks good; thanks for doing 
> this.
> 
> Can you explain exactly what it is you're trying to accomplish by
> making things volatile?  What kinds of failure modes are you expecting
> to protect against?  Glancing through the code, I can't really see
> that making things volatile will make much of a difference.

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.

> Further comments inline below.
> 
> On Tue, Jun 29, 2010 at 4:37 PM, Jan Beulich <JBeulich@xxxxxxxxxx> wrote:
>> --- 2010-06-15.orig/xen/common/trace.c  2010-06-29 17:04:45.000000000 +0200
>> +++ 2010-06-15/xen/common/trace.c       2010-06-29 17:05:09.000000000 +0200
> [snip]
>> @@ -626,13 +649,11 @@ void __trace_var(u32 event, int cycles,
>>
>>     /* Read tb_init_done /before/ t_bufs. */
>>     rmb();
>> -
>> -    spin_lock_irqsave(&this_cpu(t_lock), flags);
>> -
>>     buf = this_cpu(t_bufs);
>> -
>>     if ( unlikely(!buf) )
>> -        goto unlock;
>> +        return;
>> +
>> +    spin_lock_irqsave(&this_cpu(t_lock), flags);
> 
> This isn't any good: the whole point of this lock is to keep t_bufs
> from disappearing under our feet.

Not really - the buffer can't disappear if we make it here, it can only
disappear when tb_init_done wasn't set yet. But if that's a major
concern, I can of course put the check back into the locked region.
And wait, I think in the end there's no change needed at all -
originally I thought going to the "unlock" label here is unsafe as
the code there would de-reference buf, but started_below_highwater
still being zero would prevent this.

So I'll submit another version in any case, after we settled on
whether using volatile qualifiers is acceptable here.

Jan


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