[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] livepatch: account for patch offset when applying NOP patch
- To: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
- From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Date: Fri, 8 Apr 2022 10:19:54 +0200
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=smyQIvUoVkqdAKDiGKZM1MrDQ6rHXGCDw1hkHrsAksI=; b=PDyRN3o52JFpswG/WFYKBkKcM6iElx1uoGdbZnGqrkF3bGwdCXx/SloNQJH+cBy5cqlB2Ea/Cv3V1cL6mndwlGQ45jndJOqJ2EW/3QfhK6XYNxanSIpYQ0DkSqCzSpSc2hDMU57Mf3S3slOw+kBkreYs5IIFyANaXHq8iVZ5UjTvKBGaAx8Fm4GkavXtxcTqz2+jnCUMj2vIDFZ90jf9DCpo9F0T6BeMBW7UNW96eP2XWefN66e81moB2AglDTlaLV19avKUizwwk6C8Qa1R87px1pRyPYWlgkGAbYUk4lxvItlkUucXp08S0as2nOJvrHAWgeLTwI0u6F4X/DwxZg==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YUyvVfViTzhwtQNH6pjHgWz5ayLnFJnCJCzkbTRtYUrfhbThjSdl7qEcvbyABN8EZW+dhpW45pKIRrGfdDQGun4e5/GyqME+BtxE4qICGAHM82ckHttzOzRDAcJbrXuTFToT2e+6pdj7ghA1gatAQHUFy4PT0OryrYVpn1JFTw20m0AIPKoyerp8K8VPyJmUOjU0qyAeqy8aoaa4rObbmBYoHm2BqrXyUaCmLt+tNeENH1HNpEqi3K0R2zjxRou/3YVkfuXtmz7bzpZz73dAhPgaiZ4c6w5GWniT+ouT6CF99l6QuuFXR2suULuZfWuU1+8Iq31szgg7XRs2XL8Mwg==
- Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
- Cc: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Konrad Wilk <konrad.wilk@xxxxxxxxxx>, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Bjoern Doebel <doebel@xxxxxxxxx>
- Delivery-date: Fri, 08 Apr 2022 08:20:21 +0000
- Ironport-data: A9a23:fE9SSa0NwtThJRgIrPbD5e1xkn2cJEfYwER7XKvMYLTBsI5bpzxVn WMWWWmOPfnYMGv8Kth3YIq+oUJVsZDdzIJmTAJspC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkjk7xdOCn9xGQ7InQLlbGILes1htZGEk1EE/NtTo5w7Rj2tIw0YDga++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /0Vkd+PdVYIJ5Tjs8cWXBUBDTomBfNvreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHxO4wSoDd4xCzxBvc6W5HTBa7N4Le02R9u1poVTa+BO qL1bxJGQzSYQjIUJWsVL58fl8yQmH7fMDRh/Qf9Sa0fvDGIkV0ZPKLWGMXRUsyHQ4NShEnwj m7B8m70BjkTMdWNzjzD/n/EruzImznyVMQNFbm73vlwiVaXyyoYDxh+fVmxrOS9i0W+c8lCM EFS8S0rxYAi+UruQtTjUhmQpH+fogVaS9dWC/c96gyG1uzT+QnxO4QfZmcfMpp87pZwHGF0k A/S9z/0OdBxmOWxCk2hp5aNlByZGiMaITYcfnc8EyJQtrEPv7oPph7IS99iFou8gdv0BSz8z li2kcQuu1kApZVVjvvmpDgrlxrp/8GUFVBtum07S0r/tmtEiJiZi5tEALQxxdJJN86nQ1aIp xDocODOvblVXflheMFgKdjh/Y1FBd7YaFUwYnY1RvHNEghBHVb5IOi8BxkkeS9U3j4sI2OBX aMqkVo5CGVvFHWrd7RrRIm6Ft4ny6Ptffy8CKyFNIoTP8guK1belM2LWaJ29zqw+KTLufthU ap3jO72VSpKYUiZ5GTeqxghPU8DmXllmDK7qWHTxBW7y7uODEN5up9eWGZimtsRtfveyC2Mq o43H5LTl313Db2vCgGKoNV7BQ1bchAG6WXe9pU/mhireVE9RgnMypb5nNscRmCSt/gNzbeXo yvlAye1CjPX3BX6FOlDUVg6AJvHVpdjt3MreysqOFejwX84ZoizqqwYcvMKkXMPrYSPEdYco yE5Rvi9
- Ironport-hdrordr: A9a23:6Rts5qvpX/Ed7FCYBeOJYD8c7skClIMji2hC6mlwRA09TyXGra +TdaUguSMc1gx9ZJhBo7G90KnpewK6yXdQ2/hqAV7EZniahILIFvAY0WKG+VPd8kLFh4xgPM tbAs1D4ZjLfCRHZKXBkXiF+rQbsaC6GcmT7I+0pRcdLj2CKZsQlzuRYjzrbHGeLzM2Y6bReq Dsgvau8FGbCAsqh4mAdzE4dtmGg+eOuIPtYBYACRJiwA6SjQmw4Lq/NxSDxB8RXx5G3L9nqA H+4kbEz5Tml8v+5g7X1mfV4ZgTsNz9yuFbDMjJrsQOMD3jhiuheYwkcbyfuzIepv2p9T8R4Z LxiiZlG/42x2Laf2mzrxeo8w780Aw243un8lOciWuLm72PeBsKT+56wa5JeBrQ7EQt+Ptm1r hQ4m6fv51LSTvdgSXU/bHzJl5Xv3vxhUBnvf8YjnRZX4dbQqRWt5Yj8ERcF4pFND7m6bogDP JlAKjnlbprmGuhHjHkV1RUsZyRtixZJGbEfqFCgL3Z79FupgE286NCr/Zv3Evp9/oGOu15Dq r/Q+FVfYp1P7wrhJJGdZc8qPSMex7wqDL3QRSvyAfcZeg600ykke+D3Fxy3pDvRKA1
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On Thu, Apr 07, 2022 at 03:44:16PM +0000, Ross Lagerwall wrote:
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: Thursday, March 31, 2022 9:42 AM
> > To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Ross
> > Lagerwall <ross.lagerwall@xxxxxxxxxx>; Konrad Wilk
> > <konrad.wilk@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei
> > Liu <wl@xxxxxxx>; Bjoern Doebel <doebel@xxxxxxxxx>
> > Subject: Re: [PATCH v3] livepatch: account for patch offset when applying
> > NOP patch
> >
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments
> > unless you have verified the sender and know the content is safe.
> >
> > On 31.03.2022 10:21, Roger Pau Monné wrote:
> > > On Thu, Mar 31, 2022 at 08:49:46AM +0200, Jan Beulich wrote:
> > >> While not triggered by the trivial xen_nop in-tree patch on
> > >> staging/master, that patch exposes a problem on the stable trees, where
> > >> all functions have ENDBR inserted. When NOP-ing out a range, we need to
> > >> account for this. Handle this right in livepatch_insn_len().
> > >>
> > >> This requires livepatch_insn_len() to be called _after_ ->patch_offset
> > >> was set.
> > >>
> > >> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced
> > >> functions")
> > >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > >
> > > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >
> > Thanks.
> >
> > As a note to the livepatch maintainers: I'm going to put this in
> > without further waiting for suitable acks. Just in case I'll put
> > it on the 4.16 branch only for starters, to see that it actually
> > helps there (it's unusual to put something on the stable
> > branches before it having passed the push gate to master).
>
> Thanks (was on PTO and away from email).
>
> >
> > > Albeit I don't think I understand how the in-place patching is done. I
> > > would expect the !func->new_addr branch of the if in
> > > arch_livepatch_apply to fill the insn buffer with the in-place
> > > replacement instructions, but I only see the buffer getting filled
> > > with nops. I'm likely missing something (not that this patch changes
> > > any of this).
> >
> > Well, as per the v2 thread: There's no in-place patching except
> > to NOP out certain insns.
>
> Correct. FWIW I'm not really aware of a valid use case for this
>
> >
> > > I'm also having trouble figuring out how we assert that the len value
> > > (which is derived from new_size if !new_addr) is not greater than
> > > LIVEPATCH_OPAQUE_SIZE, which is the limit of the insn buffer. Maybe
> > > that's already checked elsewhere.
> >
> > That's what my 3rd post-commit-message remark was (partly) about.
>
> old_size specifies the length of the existing function to be patched.
>
> If new_addr is zero (NOP case), then new_size specifies the number of
> bytes to overwrite with NOP. That's why new_size is used as the memcpy
> length (minus patch offset).
Sorry, maybe a naive question, but why not use old_size directly to
overwrite with NOPs?
Is this because you could generate a patch that just removed code from
a function and then you would ideally just overwrite with NOPs the
section to be removed while leaving the rest of the function as-is (so
no jump added)?
I wonder whether we exercise this functionality at all, as I would
imagine is quite hard to come with such payload?
Thanks, Roger.
|