From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw Date: Wed, 4 Nov 2015 14:29:10 +0000 Message-ID: <1446647350.6461.89.camel@citrix.com> References: <1446228813-2593-1-git-send-email-julien.grall@citrix.com> <1446228813-2593-5-git-send-email-julien.grall@citrix.com> <1446554124.10390.19.camel@citrix.com> <5638BE43.4080607@citrix.com> <1446561267.10390.48.camel@citrix.com> <5639E949.6020800@citrix.com> <1446636465.6461.70.camel@citrix.com> <5639EEB6.6040908@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Ztz41-000237-2N for xen-devel@lists.xenproject.org; Wed, 04 Nov 2015 14:29:25 +0000 In-Reply-To: <5639EEB6.6040908@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , Stefano Stabellini Cc: xen-devel@lists.xenproject.org, Keir Fraser , Ian Jackson , Jan Beulich , Tim Deegan List-Id: xen-devel@lists.xenproject.org On Wed, 2015-11-04 at 11:40 +0000, Julien Grall wrote: > > Not sure I follow. If hnd isn't a suitable struct xen guest handle then > > other things will fail. If a user is passing a non struct > > xen_guest_handle > > to this which happens to contain the same fields then more fool them, > > and > > if it happens to be 8 bytes anyway your check won't catch that. > > With the 2 checks in set_xen_raw_guest_handle we catch most of the > problem. They both ensure that the handle is 8-byte and the pointer is > valid. However we don't check that the padding is at the beginning of > the structure. > > It's better than what we have today as we don't even check that the > handle is 8-byte. I'm not sure I'm all that worried about callers constructing their own guest handle structs and getting it wrong. > [...] > > > > > This looses out on the arm32 hypervisor sanity checking that the > > > > padding > > > > bytes are 0 (as required by the ABI) but TBH I haven't checked that > > > > the > > > > current version has that property either. > > > > > > It's done during the assignation by the compiler: > > > > > > (hnd).q = (uint64_t)(uintptr_t)(val); > > > > I meant on the reading side. > > It's the responsibility of the caller to zero the padding. There is > nothing to do on the reading side, the hypervisor will use "p" which > will be the size of the natural pointer. For a 32-bit Xen the check would be that a guest was not inadvertently violating this rule, such a guest would crash if it was run on a 64-bit hypervisor (which would see the non-zero padding as part of the pointer), by rejecting such cases on 32-bit Xen we avoid such guests becoming established and therefore presenting a case for us to relax this rule in one way or another. This is the same reason as check_multicall_32bit_clean() exists. multicall is special only in that it was pretty easy to know where to add that check. Ian.