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

[George Dunlap <dunlapg@xxxxxxxxx>]

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
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?

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.

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

> 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.

Hmm -- that happens to be the case now, but it's not guaranteed to be
that way forever, and this is essentially a "trap" laid for anyone who
changes that invariant.  At some point I would like to implement
re-sizable trace buffers, in which case the buffer may disappear after
tb_init_done is set. I'd rather not rely on my memory of this
discussion. :-)

> 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.

Yes, that's in part what setting that was for.  I'll add a comment to
that effect.

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

I've already got things in a nice patchqueue, including my modified
patches -- I'll just pull out the volatile code, make the simple
changes we've discussed, and post the patch queue.

 -George

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