| Message ID | 20230417095815.3734434-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/livepatch: Fix secure_payload() in non-debug builds | expand |
On 17.04.2023 11:58, Andrew Cooper wrote: > The ro_pages + rw_pages + text_pages != payload->pages check is not something > which is reasonable to skip at runtime. Rewrite it to not be an ASSERT(). Isn't this merely a sanity check? IOW isn't returning -EINVAL in this case misleading, as to calling "invalid input" what really is an internal error in Xen? But anyway, I guess I'll leave this to the maintainers. > As the code is being shuffled anyway, rework the logic calling > arch_livepatch_secure() to reduce its verbosity. By "verbosity", dym lines of code? Jan
> From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, April 17, 2023 11:41 AM > To: Andrew Cooper <Andrew.Cooper3@citrix.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>; Xen-devel <xen-devel@lists.xenproject.org> > Subject: Re: [PATCH] xen/livepatch: Fix secure_payload() in non-debug builds > > On 17.04.2023 11:58, Andrew Cooper wrote: > > The ro_pages + rw_pages + text_pages != payload->pages check is not something > > which is reasonable to skip at runtime. Rewrite it to not be an ASSERT(). > > Isn't this merely a sanity check? IOW isn't returning -EINVAL in this case > misleading, as to calling "invalid input" what really is an internal error > in Xen? But anyway, I guess I'll leave this to the maintainers. > Yes, it looks like it is just a sanity check of the payload->pages calculation in move_payload(). Since it is not dependent on the payload, I think ASSERT() is correct. Ross
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index d385f882c65c..c10ab1f374e0 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -405,32 +405,27 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) static int secure_payload(struct payload *payload, struct livepatch_elf *elf) { - int rc = 0; - unsigned int text_pages, rw_pages, ro_pages; + unsigned int text_pages = PFN_UP(payload->text_size); + unsigned int rw_pages = PFN_UP(payload->rw_size); + unsigned int ro_pages = PFN_UP(payload->ro_size); + int rc; - text_pages = PFN_UP(payload->text_size); + if ( ro_pages + rw_pages + text_pages != payload->pages ) + return -EINVAL; - if ( text_pages ) - { - rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX); - if ( rc ) - return rc; - } - rw_pages = PFN_UP(payload->rw_size); - if ( rw_pages ) - { - rc = arch_livepatch_secure(payload->rw_addr, rw_pages, LIVEPATCH_VA_RW); - if ( rc ) - return rc; - } + if ( text_pages && + (rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX)) ) + return rc; - ro_pages = PFN_UP(payload->ro_size); - if ( ro_pages ) - rc = arch_livepatch_secure(payload->ro_addr, ro_pages, LIVEPATCH_VA_RO); + if ( rw_pages && + (rc = arch_livepatch_secure(payload->rw_addr, rw_pages, LIVEPATCH_VA_RW)) ) + return rc; - ASSERT(ro_pages + rw_pages + text_pages == payload->pages); + if ( ro_pages && + (rc = arch_livepatch_secure(payload->ro_addr, ro_pages, LIVEPATCH_VA_RO)) ) + return rc; - return rc; + return 0; } static bool section_ok(const struct livepatch_elf *elf,
The ro_pages + rw_pages + text_pages != payload->pages check is not something which is reasonable to skip at runtime. Rewrite it to not be an ASSERT(). As the code is being shuffled anyway, rework the logic calling arch_livepatch_secure() to reduce its verbosity. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/common/livepatch.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)