[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Julien Grall <Julien.Grall@xxxxxxx>
  • Date: Tue, 15 Oct 2019 19:47:04 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Q6wHIzwmUje14Vjr2J1xsMZaMQ+++uRyRMTYR+/tt7g=; b=Z0Uqy8vGCB5KWx3nnVobmx0tg6Dr6OTnmGrRQVCzjrwCCl3FyMONmNFH20jObJprAjfW5lTjH4paRxOl2TdGdsPL1xU8taDCgtjGxAOIH8QDV/Ml9k2IgfW4u4K5adUjIPXkgBLqiL30TbhHoQ86tnCh0ZJ8aQ54nLVSahUGusr7gct6sWgOD/djhu/QH5M+tpTIF1V8qGxX1fLb+xhzNrYJkieE28T1nkLcBDk/+wZzpvFJhMk6IrHRANXINmVKUJsc+4L5gyl8ZCx9exaErU2gdDiFFOM3sst5JhnMuDdaVMVq5B4mPQGy3g6GtQnMzhTT96GGCmPaUoLK/txfLg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HGjObBlxnu0CcDglbEyEN2LTdt1NyptD+TalLszXFGarxVs3CLfmXu52FwGU2xVPQm86h+NbpMM9bkCtVEuWqm2Csbk0Wfak/IAociyrO325S6EL9P915ABQIIT73E8u7w9bvtoaGB+b0KuHybb2P01j0E3IVZ+EvGhz+xyeitBW/U+W40+Gij3CUZQdswA82XU60IHdpXkBMydIxQTDj6bmYjnfUW4Mr/BR8AeIJSRyaQK+oGbmwgpoV0zkJiz73CVznnmxK9xDtuarViKWI6osM4Qrr5FsBtUXA37LL2XPIjT7Nvj+QqMp0510FkuuKezUIrUeqUgsbFK/YV4HCg==
  • Authentication-results: spf=temperror (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; lists.xenproject.org; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;lists.xenproject.org; dmarc=none action=none header.from=arm.com;
  • Authentication-results-original: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Cc: "jgross@xxxxxxxx" <jgross@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, nd <nd@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 15 Oct 2019 19:47:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: True
  • Original-authentication-results: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Thread-index: AQHVg467lwMKdvMkWkmKN32oZFn8VqdcG4KA
  • Thread-topic: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()

Hi,

On 15/10/2019 20:28, Stefano Stabellini wrote:
> On Tue, 15 Oct 2019, Julien Grall wrote:
>> virt_to_maddr() is using the hardware page-table walk instructions to
>> translate a virtual address to physical address. The function should
>> only be called on virtual address mapped.
>>
>> _end points past the end of Xen binary and may not be mapped when the
>> binary size is page-aligned. This means virt_to_maddr() will not be able
>> to do the translation and therefore crash Xen.
>>
>> Note there is also an off-by-one issue in this code, but the panic will
>> trump that.
>>
>> Both issues can be fixed by using _end - 1 in the check.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>>
>> ---
>>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Julien Grall <julien@xxxxxxx>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Tim Deegan <tim@xxxxxxx>
>> Cc: Wei Liu <wl@xxxxxxx>
>> Cc: jgross@xxxxxxxx
>>
>> x86 seems to be affected by the off-by-one issue. Jan, Andrew?
>>
>> This could be reached by a domain via XEN_SYSCTL_page_offline_op.
>> However, the operation is not security supported (see XSA-77). So we are
>> fine here.
>> ---
>>   xen/include/asm-arm/mm.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index 262d92f18d..174acd8859 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -153,7 +153,7 @@ extern unsigned long xenheap_base_pdx;
>>   
>>   #define is_xen_fixed_mfn(mfn)                                   \
>>       ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
>> -     (mfn_to_maddr(mfn) <= virt_to_maddr(&_end)))
>> +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
> 
> Thank you for sending the patch and I think that "_end - 1" is the right
> fix. I am just wondering whether we want/need an explicit cast of some
> sort here, because technically _end is a char[] and 1 is a integer.
> Maybe:
> 
>    ((vaddr_t)_end - 1)
> 
> ?

I vaguely remember a lengthy discussion about it last year. But I don't 
think there was any conclusion in it.

In this case, what are you trying to prevent with the cast? Is it 
underflow of an array? If so, how the cast is actually going to prevent 
the compiler to do the wrong thing?

Cheers,

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.