[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 21:08:37 +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=hW1sbGOZPce1mXFgHUff2X0pjEfdL7pGeJg47XOpNtQ=; b=Ip6kDclYUNQcyz6KJBQlBKkOeO/NORk+P13nY5ePdaHsHc0qKx3pKpRAQ7rymeLs7k0+whj83DBC7fTHfXUWNVShqpkVvnTjemBA7EXCaP3/POBFAeGPn/dk63ggmfg/QBX8N8hFtJGPbOHvEa19cWeNKnFbZZfqKfwZABMf8g8AP/FR3oQp6RAfVC6yXrjgTecKssYtejPYfwbeN33R6dYAKUdaYOqJrS3AH6uJvIiIHAcP+mcA/75ww9no9WVuwyk13WNIM4H2bDJ4Wt7JTfxa7d7dK7ExfIf9QzuiGjHe9XUT1FU2+wQjMguoPLGG/tUQlXavBM14NQzj+yZh3A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ISHA2cKbkm9Cq0TQ2RoIadEckQd0bj7Po/c1UcQYv5JdHNloDfQNWZ1rquMOPTEq3S9rLs3H5faEVlxcYZFqxgzwiK2riVDuYRE5ccLB+0ckVL4vStXb1qqKPhus15ltaVvkfMD/xUOxIGrrj+VfYzminJgfI5LibKlu7v5iLXyxsILWrNkNCMN4Iy6IF1MJTGzdp8EM11Pm13lQLxXKYYULtVmN9NiPv7f9RGNMamXlWAFO+S8THwNFEjLtQSWGpXYon5f11uSdLdDR9P/YYhOs5pNH9IxCM4kqxugBON69I09EGtDtUSmftTxw6nQJRupajVxOjXZHyWbuSZHnRw==
  • 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 21:09:14 +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: AQHVg467lwMKdvMkWkmKN32oZFn8VqdcLEWA///9sACAAAhXgA==
  • Thread-topic: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()


On 15/10/2019 21:38, Stefano Stabellini wrote:
> On Tue, 15 Oct 2019, Julien Grall wrote:
>> 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?
> 
> Yes, there was a long discussion at the beginning of the year; it was
> about how to define _start and _end so that we could avoid compilers
> undefined behavior. The main underlying issue is that comparisons
> between pointers to different objects are undefined [1] (_start and _end
> can be interpreted as pointers to different objects).
> 
> This case is a bit different, and easier. The issue is that, because the
> result of "_end - 1" is not within the boundaries of the _end array,
> then the operation is "undefined" by the C specification (C99 6.5.6).
> Undefined is not good.
> 
> So, I am not really asking for any complex fix, or hypervisor-wide
> rework. I am only asking to avoid introducing new undefined behavior.
> Casting to vaddr_t should solve it I think.

I agree that we should not add more undefined behavior in Xen. However,
I don't like cast if they can't be justified.

In this particular case, you seem to be unsure this is going to remove 
an undefined behavior. IIRC, I pointed out in the past that compiler can 
see through cast.

So can we have some certainty that your suggestion is going to work?

Cheers,

> 
> [1] https://marc.info/?l=xen-devel&m=154904722227312
> 

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