Xen devel - Nov 2013 - Invalid VA => ptr conversion with xc_dom_* API after XSA-55 fox

Hi list,

Jeff and FastIce pointed out a regression between Xen 4.1.2 and 4.1.6 
when starting NetBSD domU; the kernel syms table gets slightly corrupted 
[1].

After dwelling into libxc code, FastIce noticed that changing back the 
return value to "ptr + offset" (instead of just "ptr") for 
xc_dom_vaddr_to_ptr() makes it work again.

According to [2] while fixing XSA-55, Ian changed the "ptr + offset" 
return value to just "ptr". Is there a reason for this? IMHO the VA
=>
ptr conversion should also take into account non-page aligned addresses, 
hence the offset (except for NULL value of course).

Cheers,

[1] http://mail-index.netbsd.org/port-xen/2013/10/16/msg008088.html

[2] 
http://xenbits.xen.org/gitweb/?p=xen.git;a=blobdiff;f=tools/libxc/xc_dom.h;h=ad6fdd49e2b07704c07a0629bf8da59bfdb5fef2;hp=316c5cbefca4f791255bb43f5dcf4085d42d5869;hb=b5a869209998fedadfe205d37addbd50a802998b;hpb=53bfcf585b09eb4ac2240f89d1ade77421cd2451

-- 
Jean-Yves Migeon
jym@

>>> On 20.11.13 at 13:56, "Jean-Yves Migeon (NetBSD)"
<jym@NetBSD.org> wrote:
> Jeff and FastIce pointed out a regression between Xen 4.1.2 and 4.1.6 
> when starting NetBSD domU; the kernel syms table gets slightly corrupted 
> [1].
> 
> After dwelling into libxc code, FastIce noticed that changing back the 
> return value to "ptr + offset" (instead of just "ptr")
for
> xc_dom_vaddr_to_ptr() makes it work again.
> 
> According to [2] while fixing XSA-55, Ian changed the "ptr +
offset"
> return value to just "ptr". Is there a reason for this? IMHO the
VA =>
> ptr conversion should also take into account non-page aligned addresses, 
> hence the offset (except for NULL value of course).
I agree, but let''s see what Ian (being the author of the whole
patch set) says.

Jan

Jan Beulich writes ("Re: [Xen-devel] Invalid VA => ptr conversion with
xc_dom_* API after XSA-55 fox"):
> On 20.11.13 at 13:56, "Jean-Yves Migeon (NetBSD)"
<jym@NetBSD.org> wrote:
> > Jeff and FastIce pointed out a regression between Xen 4.1.2 and 4.1.6 
> > when starting NetBSD domU; the kernel syms table gets slightly
corrupted
> > [1].
> > 
> > After dwelling into libxc code, FastIce noticed that changing back the
> > return value to "ptr + offset" (instead of just
"ptr") for
> > xc_dom_vaddr_to_ptr() makes it work again.
> > 
> > According to [2] while fixing XSA-55, Ian changed the "ptr +
offset"
> > return value to just "ptr". Is there a reason for this? IMHO
the VA =>
> > ptr conversion should also take into account non-page aligned
addresses,
> > hence the offset (except for NULL value of course).
> 
> I agree, but let''s see what Ian (being the author of the whole
> patch set) says.
It looks like I changed this in b5a86920.  Having stared at the code
and the commit message I think that I did in fact break this, by
inadvertantly removing the "+ offset".  Sorry to cause a regression.

I''ve examined the surrounding code and reread the relevant bits of
xc_dom_pfn_to_ptr_retcount and I think that simpy returning
"ptr + offset" is the correct fix.  The calculation of
*safe_region_out is already correct.

Will someone write this up as a patch submission ?

In any case, the fix ought to be backported.

Thanks,
Ian.

Le 25/11/2013 12:44, Ian Jackson a écrit :
> Jan Beulich writes ("Re: [Xen-devel] Invalid VA => ptr conversion
> with xc_dom_* API after XSA-55 fox"):
>> On 20.11.13 at 13:56, "Jean-Yves Migeon (NetBSD)"
<jym@NetBSD.org>
>> wrote:
>> > Jeff and FastIce pointed out a regression between Xen 4.1.2 and 
>> 4.1.6
>> > when starting NetBSD domU; the kernel syms table gets slightly 
>> corrupted
>> > [1].
>> >
>> > After dwelling into libxc code, FastIce noticed that changing back
>> the
>> > return value to "ptr + offset" (instead of just
"ptr") for
>> > xc_dom_vaddr_to_ptr() makes it work again.
>> >
>> > According to [2] while fixing XSA-55, Ian changed the "ptr + 
>> offset"
>> > return value to just "ptr". Is there a reason for this?
IMHO the
>> VA =>
>> > ptr conversion should also take into account non-page aligned 
>> addresses,
>> > hence the offset (except for NULL value of course).
>>
>> I agree, but let's see what Ian (being the author of the whole
>> patch set) says.
>
> It looks like I changed this in b5a86920.  Having stared at the code
> and the commit message I think that I did in fact break this, by
> inadvertantly removing the "+ offset".  Sorry to cause a
regression.
>
> I've examined the surrounding code and reread the relevant bits of
> xc_dom_pfn_to_ptr_retcount and I think that simpy returning
> "ptr + offset" is the correct fix.  The calculation of
> *safe_region_out is already correct.
>
> Will someone write this up as a patch submission ?
Will do (tonight).
> In any case, the fix ought to be backported.
>
> Thanks,
Thanks for your review!

Cheers,

-- 
Jean-Yves Migeon
jym@

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel