From patchwork Thu Sep 22 23:55:10 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Konrad Rzeszutek Wilk X-Patchwork-Id: 9347163 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6731F6077A for ; Thu, 22 Sep 2016 23:58:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 471E12AD10 for ; Thu, 22 Sep 2016 23:58:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3B6A72AD13; Thu, 22 Sep 2016 23:58:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4CB6F2AD10 for ; Thu, 22 Sep 2016 23:58:52 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bnDqO-0003wn-3c; Thu, 22 Sep 2016 23:55:56 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bnDqM-0003wb-Rc for xen-devel@lists.xenproject.org; Thu, 22 Sep 2016 23:55:54 +0000 Received: from [85.158.139.211] by server-3.bemta-5.messagelabs.com id FF/CC-01957-A8F64E75; Thu, 22 Sep 2016 23:55:54 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrLIsWRWlGSWpSXmKPExsXSO6nOVbcz/0m 4wc5tXBbft0xmcmD0OPzhCksAYxRrZl5SfkUCa0bbnqUsBVPcKlYvPM7cwHjdpIuRk0NIYCKT xPLtMV2MXED2N0aJhj1LmSGcjYwST5b8YoJwehgl7k5/wQzSwiKgKnHu/Vq2LkYODjYBE4k3q xxBwiICyhK9v36zgNjMAsUSG35uYgSxhQU8JLb3rGMHsXkFLCX693xkg5i5jVFi6o19TBAJQY mTM59ANWtJ3Pj3kglkPrOAtMTyfxwgJqeAvUTHdW6QClGgVQ0zHoBdIyFgKPF541LmCYyCs5A MmoVk0CyEQQsYmVcxahSnFpWlFukaGeglFWWmZ5TkJmbm6BoamOrlphYXJ6an5iQmFesl5+du YgQGbT0DA+MOxsbZfocYJTmYlER5u4OehAvxJeWnVGYkFmfEF5XmpBYfYpTh4FCS4H2aC5QTL EpNT61Iy8wBxg9MWoKDR0mEdwlImre4IDG3ODMdInWKUZejq/P5OSYhlrz8vFQpcd73IEUCIE UZpXlwI2CxfIlRVkqYl5GBgUGIpyC1KDezBFX+FaM4B6OSMK9mHtAUnsy8ErhNr4COYAI6Ysv PByBHlCQipKQaGPkk9Av5Ogt2crIW9WWc0mD6yj6V99iVjYWiNQZxB5qfyx3X5yx693FLe3VJ QqzXpPyWxkvn16S17Vs+ecqBs1fel+3ezLz3x+09O9yqX/6ya75iZfRnh8zNmIqKfH4vz5ptq 64vcxNaPGvdlMRQiYIr/QttPP623wu+s7Ne1OT+2dTHkw/oTlViKc5INNRiLipOBAAKmQSt4A IAAA== X-Env-Sender: konrad.wilk@oracle.com X-Msg-Ref: server-12.tower-206.messagelabs.com!1474588551!24668261!1 X-Originating-IP: [141.146.126.69] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTQxLjE0Ni4xMjYuNjkgPT4gMjc3MjE4\n X-StarScan-Received: X-StarScan-Version: 8.84; banners=-,-,- X-VirusChecked: Checked Received: (qmail 43994 invoked from network); 22 Sep 2016 23:55:52 -0000 Received: from aserp1040.oracle.com (HELO aserp1040.oracle.com) (141.146.126.69) by server-12.tower-206.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 22 Sep 2016 23:55:52 -0000 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u8MNtkMc004429 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 22 Sep 2016 23:55:47 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0022.oracle.com (8.14.4/8.13.8) with ESMTP id u8MNtjkR017198 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 22 Sep 2016 23:55:46 GMT Received: from abhmp0011.oracle.com (abhmp0011.oracle.com [141.146.116.17]) by aserv0121.oracle.com (8.13.8/8.13.8) with ESMTP id u8MNthLB027217; Thu, 22 Sep 2016 23:55:44 GMT Received: from localhost.localdomain (/209.6.196.81) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 22 Sep 2016 16:55:43 -0700 Date: Thu, 22 Sep 2016 19:55:10 -0400 From: Konrad Rzeszutek Wilk To: Jan Beulich Message-ID: <20160922235504.GA5294@localhost.localdomain> References: <1474477030-10722-1-git-send-email-konrad.wilk@oracle.com> <1474477030-10722-2-git-send-email-konrad.wilk@oracle.com> <57E3BE9C0200007800111719@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57E3BE9C0200007800111719@prv-mh.provo.novell.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Source-IP: userv0022.oracle.com [156.151.31.74] Cc: andrew.cooper3@citrix.com, xen-devel@lists.xenproject.org, ross.lagerwall@citrix.com Subject: Re: [Xen-devel] [PATCH v7 1/5] livepatch: Disallow applying after an revert X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Sep 22, 2016 at 03:21:00AM -0600, Jan Beulich wrote: > >>> On 21.09.16 at 18:57, wrote: > > @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) > > * and .shstrtab. For the non-relocate we allocate and copy these > > * via other means - and the .rel we can ignore as we only use it > > * once during loading. > > + * > > + * Also ignore sections with zero size. Those can be .data, or .bss. > > Or any others. Please make this apparent by adding "e.g." or some > such. > > > + * > > + * This logic must MATCH what is done in livepatch_elf_resolve_symbols. > > Instead of such a comment, is it perhaps worth making an inline > function or macro to cover the three instances where these > checks need to match up? > > > @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) > > > > for ( i = 1; i < elf->hdr->e_shnum; i++ ) > > { > > - if ( elf->sec[i].sec->sh_flags & SHF_ALLOC ) > > + if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size ) > > Please parenthesize the & in cases like this. Thanks! Please see the updated patch: From 829aa410c06231906b5ee786b10ede343ac39884 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Tue, 13 Sep 2016 12:02:20 -0400 Subject: [PATCH v8] livepatch: Disallow applying after an revert On general this is unhealthy - as the payload's .bss (definitly) or .data (maybe) will be modified once the payload is running. Doing an revert and then re-applying the payload with a non-pristine .bss or .data can lead to unforseen consequences (.bss are assumed to always contain zero value but now they may have a different value). There is one exception - if the payload contains only one .data section - the .livepatch.funcs, then it is OK to re-apply an revert. We detect this rather simply (if there is one RW section and its name is .livepatch.funcs) - but the payload may have many other RW sections that are not used at all (such as .bss or .data sections with zero length). To not account those we also ignore sections with sh_size being zero. Suggested-by: Jan Beulich Signed-off-by: Konrad Rzeszutek Wilk Reviewed-by: Jan Beulich --- Cc: Andrew Cooper Cc: Jan Beulich v6: New submission. v7: Use 'bool' instead of 'bool_t' - Ignore sh_size==0 sections as a way to make the exception check work. - Also add the sh_size==0 check in livepatch_elf_resolve_symbols. - Update comment in load move_payload to mention "livepatch_elf_resolve_symbols" v8: Expand the comment - Parenthesize the & - Introduce livepatch_elf_ignore_section which does the sh_size and SHF_ALLOC check --- docs/misc/livepatch.markdown | 7 +++++++ xen/common/livepatch.c | 34 +++++++++++++++++++++++++++++++--- xen/common/livepatch_elf.c | 3 +-- xen/include/xen/livepatch_elf.h | 4 ++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 89c1050..a674037 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted. That said we should provide hook functions so that the existing data can be changed during payload application. +To guarantee safety we disallow re-applying an payload after it has been +reverted. This is because we cannot guarantee that the state of .bss +and .data to be exactly as it was during loading. Hence the administrator +MUST unload the payload and upload it again to apply it. + +There is an exception to this: if the payload only has .livepatch.funcs; +and the .data or .bss sections are of zero length. ### Inline patching diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 23e4d51..912729e 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -52,6 +52,8 @@ struct livepatch_build_id { struct payload { uint32_t state; /* One of the LIVEPATCH_STATE_*. */ int32_t rc; /* 0 or -XEN_EXX. */ + bool reverted; /* Whether it was reverted. */ + bool safe_to_reapply; /* Can apply safely after revert. */ struct list_head list; /* Linked to 'payload_list'. */ const void *text_addr; /* Virtual address of .text. */ size_t text_size; /* .. and its size. */ @@ -308,7 +310,7 @@ static void calc_section(const struct livepatch_elf_sec *sec, size_t *size, static int move_payload(struct payload *payload, struct livepatch_elf *elf) { void *text_buf, *ro_buf, *rw_buf; - unsigned int i; + unsigned int i, rw_buf_sec, rw_buf_cnt = 0; size_t size = 0; unsigned int *offset; int rc = 0; @@ -325,8 +327,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) * and .shstrtab. For the non-relocate we allocate and copy these * via other means - and the .rel we can ignore as we only use it * once during loading. + * + * Also ignore sections with zero size. Those can be for example: + * data, or .bss. */ - if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) ) + if ( livepatch_elf_ignore_section(elf->sec[i].sec) ) offset[i] = UINT_MAX; else if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) && !(elf->sec[i].sec->sh_flags & SHF_WRITE) ) @@ -374,14 +379,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) for ( i = 1; i < elf->hdr->e_shnum; i++ ) { - if ( elf->sec[i].sec->sh_flags & SHF_ALLOC ) + if ( !livepatch_elf_ignore_section(elf->sec[i].sec) ) { void *buf; if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR ) buf = text_buf; else if ( elf->sec[i].sec->sh_flags & SHF_WRITE ) + { buf = rw_buf; + rw_buf_sec = i; + rw_buf_cnt++; + } else buf = ro_buf; @@ -402,6 +411,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) } } + /* Only one RW section with non-zero size: .livepatch.funcs */ + if ( rw_buf_cnt == 1 && + !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) ) + payload->safe_to_reapply = true; out: xfree(offset); @@ -1057,6 +1070,7 @@ static int revert_payload(struct payload *data) list_del_rcu(&data->applied_list); unregister_virtual_region(&data->region); + data->reverted = true; return 0; } @@ -1438,6 +1452,20 @@ static int livepatch_action(xen_sysctl_livepatch_action_t *action) case LIVEPATCH_ACTION_APPLY: if ( data->state == LIVEPATCH_STATE_CHECKED ) { + /* + * It is unsafe to apply an reverted payload as the .data (or .bss) + * may not be in in pristine condition. Hence MUST unload and then + * apply patch again. Unless the payload has only one + * RW section (.livepatch.funcs). + */ + if ( data->reverted && !data->safe_to_reapply ) + { + dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n", + LIVEPATCH, data->name); + data->rc = -EINVAL; + break; + } + rc = build_id_dep(data, !!list_empty(&applied_list)); if ( rc ) break; diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index cda9b27..6c7773b 100644 --- a/xen/common/livepatch_elf.c +++ b/xen/common/livepatch_elf.c @@ -310,8 +310,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf) break; } - /* Matches 'move_payload' which ignores such sections. */ - if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) ) + if ( livepatch_elf_ignore_section(elf->sec[idx].sec) ) break; st_value += (unsigned long)elf->sec[idx].load_addr; diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h index 7e7c86e..c3c1595 100644 --- a/xen/include/xen/livepatch_elf.h +++ b/xen/include/xen/livepatch_elf.h @@ -46,6 +46,10 @@ void livepatch_elf_free(struct livepatch_elf *elf); int livepatch_elf_resolve_symbols(struct livepatch_elf *elf); int livepatch_elf_perform_relocs(struct livepatch_elf *elf); +static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec) +{ + return ( !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0 ); +} #endif /* __XEN_LIVEPATCH_ELF_H__ */ /*