Re: [PATCH v5 06/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

From: Ross Lagerwall <ross.lagerwall@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: sstabellini@kernel.org, Andrew Cooper <andrew.cooper3@citrix.com>,
	julien.grall@arm.com, Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v5 06/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr
Date: Wed, 28 Sep 2016 11:21:31 +0100	[thread overview]
Message-ID: <9fbf9aa2-23b7-f13d-1568-973e4dbfc3a5@citrix.com> (raw)
In-Reply-To: <20160923155930.GA2652@char.us.oracle.com>

On 09/23/2016 04:59 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 23, 2016 at 11:37:27AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Fri, Sep 23, 2016 at 03:36:27PM +0100, Ross Lagerwall wrote:
>>> On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
>>>> If the distance is too big we are in trouble - as our relocation
>>>> distance can surely be clipped, or still have a valid width - but
>>>> cause an overflow of distance.
>>>>
>>>> On various architectures the maximum displacement for a unconditional
>>>> branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86
>>>> for 32-bit relocations is +/- 2G.
>>>>
>>>> Note: On x86 we could use the 64-bit jmpq instruction which
>>>> would provide much bigger displacement to do a jump, but we would
>>>> still have issues with the new function not being able to reach
>>>> any of the old functions (as all the relocations would assume 32-bit
>>>> displacement). And "furthermore would require an register or
>>>> memory location to load/store the address to." (From Jan).
>>>>
>>>> On ARM the conditional branch supports even a smaller displacement
>>>> but fortunately we are not using that.
>>>
>>> Wouldn't this be simpler as a BUILD_BUG_ON() in arch_livepatch_init()?
>>>
>>> Something like BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) -
>>> XEN_VIRT_START) > ARCH_LIVEPATCH_RANGE)?
>>>
>>> And BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - XEN_VIRT_START) >
>>> ARCH_LIVEPATCH_RANGE)
>>>
>>> And something similar for ARM...
>>
>> What does that have to with the payload? The displacement calculations(checks)
>> are done when we load the payload.
>
> Ooh, you are thinking of just making sure that the displacement in virtual
> addresses _should_ be OK.
>
> And that BUILD_BUG_ON would certainly do it.
>
> But my thinking was more of the payload either having some wacky relocations (say
> having an initial addendum that along with the XEN_VIRT_START -> XEN_VIRT_END)
> causes us to be way past the right place (especially with ARM32 where we only
> have 32MB distance). And having this extra check makes me sleep better at night.

Assuming that you had build checked the maximum possible displacement in 
virtual address space, then the only way a payload would fail the check 
is if either old_addr or new_addr doesn't point to hypervisor code or 
payload code (otherwise it would be in the range), but instead points to 
some other bit of memory. In that case, the payload is so completely 
broken you've got bigger problems...

There are many ways that payloads that can be broken. This checks only a 
special case of the payload being broken -- and only if the incorrect 
bit exceeds a certain range...

I don't really object to the check but it seems like an unnecessarily 
special case.

Cheers,
-- 
Ross Lagerwall

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