* [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
2024-09-26 10:14 [PATCH v3 0/5] xen/livepatch: improvements to loading Roger Pau Monne
@ 2024-09-26 10:14 ` Roger Pau Monne
2024-09-26 11:04 ` Andrew Cooper
` (2 more replies)
2024-09-26 10:14 ` [PATCH v3 2/5] xen/livepatch: simplify and unify logic in prepare_payload() Roger Pau Monne
` (3 subsequent siblings)
4 siblings, 3 replies; 16+ messages in thread
From: Roger Pau Monne @ 2024-09-26 10:14 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Ross Lagerwall, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Jan Beulich,
Andrew Cooper
The Elf loading logic will initially use the `data` section field to stash a
pointer to the temporary loaded data (from the buffer allocated in
livepatch_upload(), which is later relocated and the new pointer stashed in
`load_addr`.
Remove this dual field usage and use an `addr` uniformly. Initially data will
point to the temporary buffer, until relocation happens, at which point the
pointer will be updated to the relocated address.
This avoids leaking a dangling pointer in the `data` field once the temporary
buffer is freed by livepatch_upload().
Note the `addr` field cannot retain the const attribute from the previous
`data`field, as there's logic that performs manipulations against the loaded
sections, like applying relocations or sorting the exception table.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
- New in this version.
---
xen/arch/arm/arm32/livepatch.c | 8 +++---
xen/arch/arm/arm64/livepatch.c | 4 +--
xen/arch/x86/livepatch.c | 4 +--
xen/common/livepatch.c | 43 ++++++++++++++++++---------------
xen/common/livepatch_elf.c | 20 +++++++--------
xen/include/xen/livepatch_elf.h | 11 +++++----
6 files changed, 47 insertions(+), 43 deletions(-)
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index d50066564666..134d07a175bb 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -239,20 +239,20 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
if ( use_rela )
{
- const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
+ const Elf_RelA *r_a = rela->addr + i * rela->sec->sh_entsize;
symndx = ELF32_R_SYM(r_a->r_info);
type = ELF32_R_TYPE(r_a->r_info);
- dest = base->load_addr + r_a->r_offset; /* P */
+ dest = base->addr + r_a->r_offset; /* P */
addend = r_a->r_addend;
}
else
{
- const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
+ const Elf_Rel *r = rela->addr + i * rela->sec->sh_entsize;
symndx = ELF32_R_SYM(r->r_info);
type = ELF32_R_TYPE(r->r_info);
- dest = base->load_addr + r->r_offset; /* P */
+ dest = base->addr + r->r_offset; /* P */
addend = get_addend(type, dest);
}
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index dfb72be44fb8..d80051f9dc67 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -246,9 +246,9 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
{
- const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
+ const Elf_RelA *r = rela->addr + i * rela->sec->sh_entsize;
unsigned int symndx = ELF64_R_SYM(r->r_info);
- void *dest = base->load_addr + r->r_offset; /* P */
+ void *dest = base->addr + r->r_offset; /* P */
bool overflow_check = true;
int ovf = 0;
uint64_t val;
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 4f76127e1f77..be40f625d206 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -258,9 +258,9 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
{
- const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
+ const Elf_RelA *r = rela->addr + i * rela->sec->sh_entsize;
unsigned int symndx = ELF64_R_SYM(r->r_info);
- uint8_t *dest = base->load_addr + r->r_offset;
+ uint8_t *dest = base->addr + r->r_offset;
uint64_t val;
if ( symndx == STN_UNDEF )
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index df41dcce970a..7e6bf58f4408 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -371,18 +371,21 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
ASSERT(offset[i] != UINT_MAX);
- elf->sec[i].load_addr = buf + offset[i];
+ buf += offset[i];
/* Don't copy NOBITS - such as BSS. */
if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
{
- memcpy(elf->sec[i].load_addr, elf->sec[i].data,
+ memcpy(buf, elf->sec[i].addr,
elf->sec[i].sec->sh_size);
dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Loaded %s at %p\n",
- elf->name, elf->sec[i].name, elf->sec[i].load_addr);
+ elf->name, elf->sec[i].name, buf);
}
else
- memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
+ memset(buf, 0, elf->sec[i].sec->sh_size);
+
+ /* Replace the temporary buffer with the relocated one. */
+ elf->sec[i].addr = buf;
}
}
@@ -616,7 +619,7 @@ static inline int livepatch_check_expectations(const struct payload *payload)
break; \
if ( !section_ok(elf, __sec, sizeof(*hook)) || __sec->sec->sh_size != sizeof(*hook) ) \
return -EINVAL; \
- hook = __sec->load_addr; \
+ hook = __sec->addr; \
} while (0)
/*
@@ -630,7 +633,7 @@ static inline int livepatch_check_expectations(const struct payload *payload)
break; \
if ( !section_ok(elf, __sec, sizeof(*hook)) ) \
return -EINVAL; \
- hook = __sec->load_addr; \
+ hook = __sec->addr; \
nhooks = __sec->sec->sh_size / sizeof(*hook); \
} while (0)
@@ -650,7 +653,7 @@ static int prepare_payload(struct payload *payload,
if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
return -EINVAL;
- payload->funcs = funcs = sec->load_addr;
+ payload->funcs = funcs = sec->addr;
payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
payload->fstate = xzalloc_array(typeof(*payload->fstate),
@@ -709,7 +712,7 @@ static int prepare_payload(struct payload *payload,
{
const struct payload *data;
- n = sec->load_addr;
+ n = sec->addr;
if ( sec->sec->sh_size <= sizeof(*n) )
return -EINVAL;
@@ -739,7 +742,7 @@ static int prepare_payload(struct payload *payload,
sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS);
if ( sec )
{
- n = sec->load_addr;
+ n = sec->addr;
if ( sec->sec->sh_size <= sizeof(*n) )
return -EINVAL;
@@ -755,7 +758,7 @@ static int prepare_payload(struct payload *payload,
sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
if ( sec )
{
- n = sec->load_addr;
+ n = sec->addr;
if ( sec->sec->sh_size <= sizeof(*n) )
return -EINVAL;
@@ -794,8 +797,8 @@ static int prepare_payload(struct payload *payload,
if ( !section_ok(elf, sec, sizeof(*region->frame[i].start)) )
return -EINVAL;
- region->frame[i].start = sec->load_addr;
- region->frame[i].stop = sec->load_addr + sec->sec->sh_size;
+ region->frame[i].start = sec->addr;
+ region->frame[i].stop = sec->addr + sec->sec->sh_size;
}
sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
@@ -843,8 +846,8 @@ static int prepare_payload(struct payload *payload,
return -EINVAL;
}
- start = sec->load_addr;
- end = sec->load_addr + sec->sec->sh_size;
+ start = sec->addr;
+ end = sec->addr + sec->sec->sh_size;
for ( a = start; a < end; a++ )
{
@@ -867,14 +870,14 @@ static int prepare_payload(struct payload *payload,
* repl must be fully within .altinstr_replacement, even if the
* replacement and the section happen to both have zero length.
*/
- if ( repl < repl_sec->load_addr ||
+ if ( repl < repl_sec->addr ||
a->repl_len > repl_sec->sec->sh_size ||
- repl + a->repl_len > repl_sec->load_addr + repl_sec->sec->sh_size )
+ repl + a->repl_len > repl_sec->addr + repl_sec->sec->sh_size )
{
printk(XENLOG_ERR LIVEPATCH
"%s Alternative repl %p+%#x outside .altinstr_replacement %p+%#"PRIxElfWord"\n",
elf->name, repl, a->repl_len,
- repl_sec->load_addr, repl_sec->sec->sh_size);
+ repl_sec->addr, repl_sec->sec->sh_size);
return -EINVAL;
}
}
@@ -896,8 +899,8 @@ static int prepare_payload(struct payload *payload,
if ( !section_ok(elf, sec, sizeof(*region->ex)) )
return -EINVAL;
- s = sec->load_addr;
- e = sec->load_addr + sec->sec->sh_size;
+ s = sec->addr;
+ e = sec->addr + sec->sec->sh_size;
sort_exception_table(s ,e);
@@ -916,7 +919,7 @@ static int prepare_payload(struct payload *payload,
if ( !section_ok(elf, sec, sizeof(*payload->metadata.data)) )
return -EINVAL;
- payload->metadata.data = sec->load_addr;
+ payload->metadata.data = sec->addr;
payload->metadata.len = sec->sec->sh_size;
/* The metadata is required to consists of null terminated strings. */
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 45d73912a3cd..25ce1bd5a0ad 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -36,7 +36,7 @@ static int elf_verify_strtab(const struct livepatch_elf_sec *sec)
if ( !s->sh_size )
return -EINVAL;
- contents = sec->data;
+ contents = sec->addr;
if ( contents[0] || contents[s->sh_size - 1] )
return -EINVAL;
@@ -44,7 +44,7 @@ static int elf_verify_strtab(const struct livepatch_elf_sec *sec)
return 0;
}
-static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
+static int elf_resolve_sections(struct livepatch_elf *elf, void *data)
{
struct livepatch_elf_sec *sec;
unsigned int i;
@@ -104,7 +104,7 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
sec[i].sec->sh_size > LIVEPATCH_MAX_SIZE )
return -EINVAL;
- sec[i].data = data + delta;
+ sec[i].addr = data + delta;
/* Name is populated in elf_resolve_section_names. */
sec[i].name = NULL;
@@ -226,14 +226,14 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
strtab_sec = elf->strtab;
/* Pointers arithmetic to get file offset. */
- offset = strtab_sec->data - data;
+ offset = strtab_sec->addr - data;
/* Checked already in elf_resolve_sections, but just in case. */
ASSERT(offset == strtab_sec->sec->sh_offset);
ASSERT(offset < elf->len && (offset + strtab_sec->sec->sh_size <= elf->len));
- /* symtab_sec->data was computed in elf_resolve_sections. */
- ASSERT((symtab_sec->sec->sh_offset + data) == symtab_sec->data);
+ /* symtab_sec->addr was computed in elf_resolve_sections. */
+ ASSERT((symtab_sec->sec->sh_offset + data) == symtab_sec->addr);
/* No need to check values as elf_resolve_sections did it. */
nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
@@ -251,7 +251,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
for ( i = 1; i < nsym; i++ )
{
- const Elf_Sym *s = symtab_sec->data + symtab_sec->sec->sh_entsize * i;
+ const Elf_Sym *s = symtab_sec->addr + symtab_sec->sec->sh_entsize * i;
delta = s->st_name;
/* Boundary check within the .strtab. */
@@ -263,7 +263,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
}
sym[i].sym = s;
- sym[i].name = strtab_sec->data + delta;
+ sym[i].name = strtab_sec->addr + delta;
if ( arch_livepatch_symbol_deny(elf, &sym[i]) )
{
printk(XENLOG_ERR LIVEPATCH "%s: Symbol '%s' should not be in payload\n",
@@ -342,7 +342,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
break;
}
- st_value += (unsigned long)elf->sec[idx].load_addr;
+ st_value += (unsigned long)elf->sec[idx].addr;
if ( elf->sym[i].name )
dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Symbol resolved: %s => %#"PRIxElfAddr" (%s)\n",
elf->name, elf->sym[i].name,
@@ -503,7 +503,7 @@ static int livepatch_header_check(const struct livepatch_elf *elf)
return 0;
}
-int livepatch_elf_load(struct livepatch_elf *elf, const void *data)
+int livepatch_elf_load(struct livepatch_elf *elf, void *data)
{
int rc;
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 7116deaddc28..842111e14518 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -13,10 +13,11 @@ struct livepatch_elf_sec {
const Elf_Shdr *sec; /* Hooked up in elf_resolve_sections.*/
const char *name; /* Human readable name hooked in
elf_resolve_section_names. */
- const void *data; /* Pointer to the section (done by
- elf_resolve_sections). */
- void *load_addr; /* A pointer to the allocated destination.
- Done by load_payload_data. */
+ void *addr; /*
+ * Pointer to the section. This is
+ * first a temporary buffer, then
+ * later the relocated load address.
+ */
};
struct livepatch_elf_sym {
@@ -41,7 +42,7 @@ struct livepatch_elf {
const struct livepatch_elf_sec *
livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
const char *name);
-int livepatch_elf_load(struct livepatch_elf *elf, const void *data);
+int livepatch_elf_load(struct livepatch_elf *elf, void *data);
void livepatch_elf_free(struct livepatch_elf *elf);
int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
2024-09-26 10:14 ` [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field Roger Pau Monne
@ 2024-09-26 11:04 ` Andrew Cooper
2024-09-26 11:27 ` Roger Pau Monné
2024-09-26 12:35 ` Michal Orzel
2024-09-26 15:35 ` Ross Lagerwall
2 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2024-09-26 11:04 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Ross Lagerwall, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Jan Beulich
On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> The Elf loading logic will initially use the `data` section field to stash a
> pointer to the temporary loaded data (from the buffer allocated in
> livepatch_upload(), which is later relocated and the new pointer stashed in
> `load_addr`.
>
> Remove this dual field usage and use an `addr` uniformly. Initially data will
> point to the temporary buffer, until relocation happens, at which point the
> pointer will be updated to the relocated address.
>
> This avoids leaking a dangling pointer in the `data` field once the temporary
> buffer is freed by livepatch_upload().
>
> Note the `addr` field cannot retain the const attribute from the previous
> `data`field, as there's logic that performs manipulations against the loaded
> sections, like applying relocations or sorting the exception table.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index df41dcce970a..7e6bf58f4408 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -371,18 +371,21 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>
> ASSERT(offset[i] != UINT_MAX);
>
> - elf->sec[i].load_addr = buf + offset[i];
> + buf += offset[i];
>
> /* Don't copy NOBITS - such as BSS. */
> if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
> {
> - memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> + memcpy(buf, elf->sec[i].addr,
> elf->sec[i].sec->sh_size);
> dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Loaded %s at %p\n",
> - elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> + elf->name, elf->sec[i].name, buf);
> }
> else
> - memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> + memset(buf, 0, elf->sec[i].sec->sh_size);
> +
> + /* Replace the temporary buffer with the relocated one. */
> + elf->sec[i].addr = buf;
I'd suggest /* Update sec[] to refer to its final location. */
Replace is technically the memcpy() above, and "relocate" means
something else in ELF terms.
Can fix on commit.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
2024-09-26 11:04 ` Andrew Cooper
@ 2024-09-26 11:27 ` Roger Pau Monné
0 siblings, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2024-09-26 11:27 UTC (permalink / raw)
To: Andrew Cooper
Cc: xen-devel, Ross Lagerwall, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Jan Beulich
On Thu, Sep 26, 2024 at 12:04:06PM +0100, Andrew Cooper wrote:
> On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> > The Elf loading logic will initially use the `data` section field to stash a
> > pointer to the temporary loaded data (from the buffer allocated in
> > livepatch_upload(), which is later relocated and the new pointer stashed in
> > `load_addr`.
> >
> > Remove this dual field usage and use an `addr` uniformly. Initially data will
> > point to the temporary buffer, until relocation happens, at which point the
> > pointer will be updated to the relocated address.
> >
> > This avoids leaking a dangling pointer in the `data` field once the temporary
> > buffer is freed by livepatch_upload().
> >
> > Note the `addr` field cannot retain the const attribute from the previous
> > `data`field, as there's logic that performs manipulations against the loaded
> > sections, like applying relocations or sorting the exception table.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index df41dcce970a..7e6bf58f4408 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -371,18 +371,21 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >
> > ASSERT(offset[i] != UINT_MAX);
> >
> > - elf->sec[i].load_addr = buf + offset[i];
> > + buf += offset[i];
> >
> > /* Don't copy NOBITS - such as BSS. */
> > if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
> > {
> > - memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> > + memcpy(buf, elf->sec[i].addr,
> > elf->sec[i].sec->sh_size);
> > dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Loaded %s at %p\n",
> > - elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> > + elf->name, elf->sec[i].name, buf);
> > }
> > else
> > - memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > + memset(buf, 0, elf->sec[i].sec->sh_size);
> > +
> > + /* Replace the temporary buffer with the relocated one. */
> > + elf->sec[i].addr = buf;
>
> I'd suggest /* Update sec[] to refer to its final location. */
>
> Replace is technically the memcpy() above, and "relocate" means
> something else in ELF terms.
Sure, no strong opinion.
> Can fix on commit.
Thanks!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
2024-09-26 10:14 ` [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field Roger Pau Monne
2024-09-26 11:04 ` Andrew Cooper
@ 2024-09-26 12:35 ` Michal Orzel
2024-09-26 15:35 ` Ross Lagerwall
2 siblings, 0 replies; 16+ messages in thread
From: Michal Orzel @ 2024-09-26 12:35 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Ross Lagerwall, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Jan Beulich, Andrew Cooper
On 26/09/2024 12:14, Roger Pau Monne wrote:
>
>
> The Elf loading logic will initially use the `data` section field to stash a
> pointer to the temporary loaded data (from the buffer allocated in
> livepatch_upload(), which is later relocated and the new pointer stashed in
> `load_addr`.
>
> Remove this dual field usage and use an `addr` uniformly. Initially data will
> point to the temporary buffer, until relocation happens, at which point the
> pointer will be updated to the relocated address.
>
> This avoids leaking a dangling pointer in the `data` field once the temporary
> buffer is freed by livepatch_upload().
>
> Note the `addr` field cannot retain the const attribute from the previous
> `data`field, as there's logic that performs manipulations against the loaded
> sections, like applying relocations or sorting the exception table.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
For Arm part:
Acked-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
2024-09-26 10:14 ` [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field Roger Pau Monne
2024-09-26 11:04 ` Andrew Cooper
2024-09-26 12:35 ` Michal Orzel
@ 2024-09-26 15:35 ` Ross Lagerwall
2 siblings, 0 replies; 16+ messages in thread
From: Ross Lagerwall @ 2024-09-26 15:35 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Jan Beulich, Andrew Cooper
On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> The Elf loading logic will initially use the `data` section field to stash a
> pointer to the temporary loaded data (from the buffer allocated in
> livepatch_upload(), which is later relocated and the new pointer stashed in
> `load_addr`.
>
> Remove this dual field usage and use an `addr` uniformly. Initially data will
> point to the temporary buffer, until relocation happens, at which point the
> pointer will be updated to the relocated address.
>
> This avoids leaking a dangling pointer in the `data` field once the temporary
> buffer is freed by livepatch_upload().
>
> Note the `addr` field cannot retain the const attribute from the previous
> `data`field, as there's logic that performs manipulations against the loaded
> sections, like applying relocations or sorting the exception table.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/5] xen/livepatch: simplify and unify logic in prepare_payload()
2024-09-26 10:14 [PATCH v3 0/5] xen/livepatch: improvements to loading Roger Pau Monne
2024-09-26 10:14 ` [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field Roger Pau Monne
@ 2024-09-26 10:14 ` Roger Pau Monne
2024-09-26 15:52 ` Ross Lagerwall
2024-09-26 10:14 ` [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier Roger Pau Monne
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monne @ 2024-09-26 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Ross Lagerwall, Andrew Cooper
The following sections: .note.gnu.build-id, .livepatch.xen_depends and
.livepatch.depends are mandatory and ensured to be present by
check_special_sections() before prepare_payload() is called.
Simplify the logic in prepare_payload() by introducing a generic function to
parse the sections that contain a buildid. Note the function assumes the
buildid related section to always be present.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- Rename.
- Change order of assert.
---
xen/common/livepatch.c | 110 +++++++++++++++++++----------------------
1 file changed, 50 insertions(+), 60 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 7e6bf58f4408..50e2268e19a3 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -470,6 +470,31 @@ static int xen_build_id_dep(const struct payload *payload)
return 0;
}
+/* Parses build-id sections into the given destination. */
+static int parse_buildid(const struct livepatch_elf_sec *sec,
+ struct livepatch_build_id *id)
+{
+ const Elf_Note *n;
+ int rc;
+
+ /* Presence of the sections is ensured by check_special_sections(). */
+ ASSERT(sec);
+
+ n = sec->addr;
+
+ if ( sec->sec->sh_size <= sizeof(*n) )
+ return -EINVAL;
+
+ rc = xen_build_id_check(n, sec->sec->sh_size, &id->p, &id->len);
+ if ( rc )
+ return rc;
+
+ if ( !id->len || !id->p )
+ return -EINVAL;
+
+ return 0;
+}
+
static int check_special_sections(const struct livepatch_elf *elf)
{
unsigned int i;
@@ -641,11 +666,12 @@ static int prepare_payload(struct payload *payload,
struct livepatch_elf *elf)
{
const struct livepatch_elf_sec *sec;
+ const struct payload *data;
unsigned int i;
struct livepatch_func *funcs;
struct livepatch_func *f;
struct virtual_region *region;
- const Elf_Note *n;
+ int rc;
sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
if ( sec )
@@ -663,8 +689,6 @@ static int prepare_payload(struct payload *payload,
for ( i = 0; i < payload->nfuncs; i++ )
{
- int rc;
-
f = &(funcs[i]);
if ( f->version != LIVEPATCH_PAYLOAD_VERSION )
@@ -707,69 +731,35 @@ static int prepare_payload(struct payload *payload,
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.action, ELF_LIVEPATCH_REVERT_HOOK);
LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ELF_LIVEPATCH_POSTREVERT_HOOK);
- sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
- if ( sec )
- {
- const struct payload *data;
-
- n = sec->addr;
-
- if ( sec->sec->sh_size <= sizeof(*n) )
- return -EINVAL;
-
- if ( xen_build_id_check(n, sec->sec->sh_size,
- &payload->id.p, &payload->id.len) )
- return -EINVAL;
-
- if ( !payload->id.len || !payload->id.p )
- return -EINVAL;
+ rc = parse_buildid(livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE),
+ &payload->id);
+ if ( rc )
+ return rc;
- /* Make sure it is not a duplicate. */
- list_for_each_entry ( data, &payload_list, list )
+ /* Make sure it is not a duplicate. */
+ list_for_each_entry ( data, &payload_list, list )
+ {
+ /* No way _this_ payload is on the list. */
+ ASSERT(data != payload);
+ if ( data->id.len == payload->id.len &&
+ !memcmp(data->id.p, payload->id.p, data->id.len) )
{
- /* No way _this_ payload is on the list. */
- ASSERT(data != payload);
- if ( data->id.len == payload->id.len &&
- !memcmp(data->id.p, payload->id.p, data->id.len) )
- {
- dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Already loaded as %s!\n",
- elf->name, data->name);
- return -EEXIST;
- }
+ dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Already loaded as %s!\n",
+ elf->name, data->name);
+ return -EEXIST;
}
}
- sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS);
- if ( sec )
- {
- n = sec->addr;
-
- if ( sec->sec->sh_size <= sizeof(*n) )
- return -EINVAL;
-
- if ( xen_build_id_check(n, sec->sec->sh_size,
- &payload->dep.p, &payload->dep.len) )
- return -EINVAL;
-
- if ( !payload->dep.len || !payload->dep.p )
- return -EINVAL;
- }
-
- sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
- if ( sec )
- {
- n = sec->addr;
-
- if ( sec->sec->sh_size <= sizeof(*n) )
- return -EINVAL;
-
- if ( xen_build_id_check(n, sec->sec->sh_size,
- &payload->xen_dep.p, &payload->xen_dep.len) )
- return -EINVAL;
+ rc = parse_buildid(livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS),
+ &payload->dep);
+ if ( rc )
+ return rc;
- if ( !payload->xen_dep.len || !payload->xen_dep.p )
- return -EINVAL;
- }
+ rc = parse_buildid(livepatch_elf_sec_by_name(elf,
+ ELF_LIVEPATCH_XEN_DEPENDS),
+ &payload->xen_dep);
+ if ( rc )
+ return rc;
/* Setup the virtual region with proper data. */
region = &payload->region;
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/5] xen/livepatch: simplify and unify logic in prepare_payload()
2024-09-26 10:14 ` [PATCH v3 2/5] xen/livepatch: simplify and unify logic in prepare_payload() Roger Pau Monne
@ 2024-09-26 15:52 ` Ross Lagerwall
0 siblings, 0 replies; 16+ messages in thread
From: Ross Lagerwall @ 2024-09-26 15:52 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Andrew Cooper
On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> The following sections: .note.gnu.build-id, .livepatch.xen_depends and
> .livepatch.depends are mandatory and ensured to be present by
> check_special_sections() before prepare_payload() is called.
>
> Simplify the logic in prepare_payload() by introducing a generic function to
> parse the sections that contain a buildid. Note the function assumes the
> buildid related section to always be present.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier
2024-09-26 10:14 [PATCH v3 0/5] xen/livepatch: improvements to loading Roger Pau Monne
2024-09-26 10:14 ` [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field Roger Pau Monne
2024-09-26 10:14 ` [PATCH v3 2/5] xen/livepatch: simplify and unify logic in prepare_payload() Roger Pau Monne
@ 2024-09-26 10:14 ` Roger Pau Monne
2024-09-26 11:06 ` Andrew Cooper
2024-09-26 16:11 ` Ross Lagerwall
2024-09-26 10:14 ` [PATCH v3 4/5] x86/alternatives: do not BUG during apply Roger Pau Monne
2024-09-26 10:14 ` [PATCH v3 5/5] x86/alternative: build time check feature is in range Roger Pau Monne
4 siblings, 2 replies; 16+ messages in thread
From: Roger Pau Monne @ 2024-09-26 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Ross Lagerwall
The check against the expected Xen build ID should be done ahead of attempting
to apply the alternatives contained in the livepatch.
If the CPUID in the alternatives patching data is out of the scope of the
running Xen featureset the BUG() in _apply_alternatives() will trigger thus
bringing the system down. Note the layout of struct alt_instr could also
change between versions. It's also possible for struct exception_table_entry
to have changed format, hence leading to other kind of errors if parsing of the
payload is done ahead of checking if the Xen build-id matches.
Move the Xen build ID check as early as possible. To do so introduce a new
check_xen_buildid() function that parses and checks the Xen build-id before
moving the payload. Since the expected Xen build-id is used early to
detect whether the livepatch payload could be loaded, there's no reason to
store it in the payload struct, as a non-matching Xen build-id won't get the
payload populated in the first place.
Note printing the expected Xen build ID has part of dumping the payload
information is no longer done: all loaded payloads would have Xen build IDs
matching the running Xen, otherwise they would have failed to load.
Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
- Move contents of xen_build_id_dep() into check_xen_buildid().
Changes since v1:
- Do the Xen build-id check even earlier.
---
xen/common/livepatch.c | 86 +++++++++++++++++------------
xen/include/xen/livepatch_payload.h | 1 -
2 files changed, 50 insertions(+), 37 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 50e2268e19a3..f7db4be96e66 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -448,28 +448,6 @@ static bool section_ok(const struct livepatch_elf *elf,
return true;
}
-static int xen_build_id_dep(const struct payload *payload)
-{
- const void *id = NULL;
- unsigned int len = 0;
- int rc;
-
- ASSERT(payload->xen_dep.len);
- ASSERT(payload->xen_dep.p);
-
- rc = xen_build_id(&id, &len);
- if ( rc )
- return rc;
-
- if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len) ) {
- printk(XENLOG_ERR LIVEPATCH "%s: check against hypervisor build-id failed\n",
- payload->name);
- return -EINVAL;
- }
-
- return 0;
-}
-
/* Parses build-id sections into the given destination. */
static int parse_buildid(const struct livepatch_elf_sec *sec,
struct livepatch_build_id *id)
@@ -495,11 +473,56 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
return 0;
}
+static int check_xen_buildid(const struct livepatch_elf *elf)
+{
+ const void *id;
+ unsigned int len;
+ struct livepatch_build_id lp_id;
+ const struct livepatch_elf_sec *sec =
+ livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
+ int rc;
+
+ if ( !sec )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: section %s is missing\n",
+ elf->name, ELF_LIVEPATCH_XEN_DEPENDS);
+ return -EINVAL;
+ }
+
+ rc = parse_buildid(sec, &lp_id);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH
+ "%s: failed to parse section %s as build-id: %d\n",
+ elf->name, ELF_LIVEPATCH_XEN_DEPENDS, rc);
+ return -EINVAL;
+ }
+
+ rc = xen_build_id(&id, &len);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH
+ "%s: unable to get running Xen build-id: %d\n",
+ elf->name, rc);
+ return rc;
+ }
+
+ if ( lp_id.len != len || memcmp(id, lp_id.p, len) )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: build-id mismatch:\n"
+ " livepatch: %*phN\n"
+ " xen: %*phN\n",
+ elf->name, lp_id.len, lp_id.p, len, id);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int check_special_sections(const struct livepatch_elf *elf)
{
unsigned int i;
static const char *const names[] = { ELF_LIVEPATCH_DEPENDS,
- ELF_LIVEPATCH_XEN_DEPENDS,
ELF_BUILD_ID_NOTE};
for ( i = 0; i < ARRAY_SIZE(names); i++ )
@@ -755,12 +778,6 @@ static int prepare_payload(struct payload *payload,
if ( rc )
return rc;
- rc = parse_buildid(livepatch_elf_sec_by_name(elf,
- ELF_LIVEPATCH_XEN_DEPENDS),
- &payload->xen_dep);
- if ( rc )
- return rc;
-
/* Setup the virtual region with proper data. */
region = &payload->region;
@@ -1069,6 +1086,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
if ( rc )
goto out;
+ rc = check_xen_buildid(&elf);
+ if ( rc )
+ goto out;
+
rc = move_payload(payload, &elf);
if ( rc )
goto out;
@@ -1093,10 +1114,6 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
if ( rc )
goto out;
- rc = xen_build_id_dep(payload);
- if ( rc )
- goto out;
-
rc = build_symbol_table(payload, &elf);
if ( rc )
goto out;
@@ -2199,9 +2216,6 @@ static void cf_check livepatch_printall(unsigned char key)
if ( data->dep.len )
printk("depend-on=%*phN\n", data->dep.len, data->dep.p);
-
- if ( data->xen_dep.len )
- printk("depend-on-xen=%*phN\n", data->xen_dep.len, data->xen_dep.p);
}
spin_unlock(&payload_lock);
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 472d6a4a63c1..c6dc7cb5fa21 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -62,7 +62,6 @@ struct payload {
unsigned int nsyms; /* Nr of entries in .strtab and symbols. */
struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */
- struct livepatch_build_id xen_dep; /* ELFNOTE_DESC(.livepatch.xen_depends). */
livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
struct livepatch_hooks hooks; /* Pre and post hooks for apply and revert */
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier
2024-09-26 10:14 ` [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier Roger Pau Monne
@ 2024-09-26 11:06 ` Andrew Cooper
2024-09-26 16:11 ` Ross Lagerwall
1 sibling, 0 replies; 16+ messages in thread
From: Andrew Cooper @ 2024-09-26 11:06 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Ross Lagerwall
On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> The check against the expected Xen build ID should be done ahead of attempting
> to apply the alternatives contained in the livepatch.
>
> If the CPUID in the alternatives patching data is out of the scope of the
> running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> bringing the system down. Note the layout of struct alt_instr could also
> change between versions. It's also possible for struct exception_table_entry
> to have changed format, hence leading to other kind of errors if parsing of the
> payload is done ahead of checking if the Xen build-id matches.
>
> Move the Xen build ID check as early as possible. To do so introduce a new
> check_xen_buildid() function that parses and checks the Xen build-id before
> moving the payload. Since the expected Xen build-id is used early to
> detect whether the livepatch payload could be loaded, there's no reason to
> store it in the payload struct, as a non-matching Xen build-id won't get the
> payload populated in the first place.
>
> Note printing the expected Xen build ID has part of dumping the payload
> information is no longer done: all loaded payloads would have Xen build IDs
> matching the running Xen, otherwise they would have failed to load.
>
> Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier
2024-09-26 10:14 ` [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier Roger Pau Monne
2024-09-26 11:06 ` Andrew Cooper
@ 2024-09-26 16:11 ` Ross Lagerwall
2024-09-26 16:59 ` Roger Pau Monné
1 sibling, 1 reply; 16+ messages in thread
From: Ross Lagerwall @ 2024-09-26 16:11 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> The check against the expected Xen build ID should be done ahead of attempting
> to apply the alternatives contained in the livepatch.
>
> If the CPUID in the alternatives patching data is out of the scope of the
> running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> bringing the system down. Note the layout of struct alt_instr could also
> change between versions. It's also possible for struct exception_table_entry
> to have changed format, hence leading to other kind of errors if parsing of the
> payload is done ahead of checking if the Xen build-id matches.
>
> Move the Xen build ID check as early as possible. To do so introduce a new
> check_xen_buildid() function that parses and checks the Xen build-id before
> moving the payload. Since the expected Xen build-id is used early to
> detect whether the livepatch payload could be loaded, there's no reason to
> store it in the payload struct, as a non-matching Xen build-id won't get the
> payload populated in the first place.
>
> Note printing the expected Xen build ID has part of dumping the payload
> information is no longer done: all loaded payloads would have Xen build IDs
> matching the running Xen, otherwise they would have failed to load.
>
> Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Should the ELF_LIVEPATCH_XEN_DEPENDS check also be dropped from
check_special_sections() since it is no longer necessary?
Ross
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier
2024-09-26 16:11 ` Ross Lagerwall
@ 2024-09-26 16:59 ` Roger Pau Monné
0 siblings, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2024-09-26 16:59 UTC (permalink / raw)
To: Ross Lagerwall; +Cc: xen-devel
On Thu, Sep 26, 2024 at 05:11:19PM +0100, Ross Lagerwall wrote:
> On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > The check against the expected Xen build ID should be done ahead of attempting
> > to apply the alternatives contained in the livepatch.
> >
> > If the CPUID in the alternatives patching data is out of the scope of the
> > running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> > bringing the system down. Note the layout of struct alt_instr could also
> > change between versions. It's also possible for struct exception_table_entry
> > to have changed format, hence leading to other kind of errors if parsing of the
> > payload is done ahead of checking if the Xen build-id matches.
> >
> > Move the Xen build ID check as early as possible. To do so introduce a new
> > check_xen_buildid() function that parses and checks the Xen build-id before
> > moving the payload. Since the expected Xen build-id is used early to
> > detect whether the livepatch payload could be loaded, there's no reason to
> > store it in the payload struct, as a non-matching Xen build-id won't get the
> > payload populated in the first place.
> >
> > Note printing the expected Xen build ID has part of dumping the payload
> > information is no longer done: all loaded payloads would have Xen build IDs
> > matching the running Xen, otherwise they would have failed to load.
> >
> > Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Should the ELF_LIVEPATCH_XEN_DEPENDS check also be dropped from
> check_special_sections() since it is no longer necessary?
It's dropped from check_special_sections() in this patch, just not
mentioned in the commit message I'm afraid.
Thanks, Roger.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 4/5] x86/alternatives: do not BUG during apply
2024-09-26 10:14 [PATCH v3 0/5] xen/livepatch: improvements to loading Roger Pau Monne
` (2 preceding siblings ...)
2024-09-26 10:14 ` [PATCH v3 3/5] xen/livepatch: do Xen build-id check earlier Roger Pau Monne
@ 2024-09-26 10:14 ` Roger Pau Monne
2024-09-26 11:09 ` Andrew Cooper
2024-09-26 10:14 ` [PATCH v3 5/5] x86/alternative: build time check feature is in range Roger Pau Monne
4 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monne @ 2024-09-26 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Ross Lagerwall
alternatives is used both at boot time, and when loading livepatch payloads.
While for the former it makes sense to panic, it's not useful for the later, as
for livepatches it's possible to fail to load the livepatch if alternatives
cannot be resolved and continue operating normally.
Relax the BUGs in _apply_alternatives() to instead return an error code. The
caller will figure out whether the failures are fatal and panic.
Print an error message to provide some user-readable information about what
went wrong.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v2:
- Improve log messages.
Changes since v1:
- Unconditionally return from _apply_alternative() and let the caller panic
if required.
- Remove test, as next patch imposes restrictions that break the test.
---
xen/arch/x86/alternative.c | 46 ++++++++++++++++++++------
xen/arch/x86/include/asm/alternative.h | 2 +-
xen/common/livepatch.c | 10 +++++-
3 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 7824053c9d33..990b7c600932 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -175,9 +175,9 @@ extern void *const __initdata_cf_clobber_end[];
* invocation, such that no CALLs/JMPs to NULL pointers will be left
* around. See also the further comment below.
*/
-static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
- struct alt_instr *end,
- bool force)
+static int init_or_livepatch _apply_alternatives(struct alt_instr *start,
+ struct alt_instr *end,
+ bool force)
{
struct alt_instr *a, *base;
@@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
uint8_t buf[MAX_PATCH_LEN];
unsigned int total_len = a->orig_len + a->pad_len;
- BUG_ON(a->repl_len > total_len);
- BUG_ON(total_len > sizeof(buf));
- BUG_ON(a->cpuid >= NCAPINTS * 32);
+ if ( a->repl_len > total_len )
+ {
+ printk(XENLOG_ERR
+ "alt for %ps, replacement size %#x larger than origin %#x\n",
+ ALT_ORIG_PTR(a), a->repl_len, total_len);
+ return -ENOSPC;
+ }
+
+ if ( total_len > sizeof(buf) )
+ {
+ printk(XENLOG_ERR
+ "alt for %ps, origin size %#x bigger than buffer %#zx\n",
+ ALT_ORIG_PTR(a), total_len, sizeof(buf));
+ return -ENOSPC;
+ }
+
+ if ( a->cpuid >= NCAPINTS * 32 )
+ {
+ printk(XENLOG_ERR
+ "alt for %ps, feature %#x outside of featureset range %#x\n",
+ ALT_ORIG_PTR(a), a->cpuid, NCAPINTS * 32);
+ return -ERANGE;
+ }
/*
* Detect sequences of alt_instr's patching the same origin site, and
@@ -356,12 +376,14 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);
}
+
+ return 0;
}
#ifdef CONFIG_LIVEPATCH
-void apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+int apply_alternatives(struct alt_instr *start, struct alt_instr *end)
{
- _apply_alternatives(start, end, true);
+ return _apply_alternatives(start, end, true);
}
#endif
@@ -383,6 +405,8 @@ static int __init cf_check nmi_apply_alternatives(
*/
if ( !(alt_done & alt_todo) )
{
+ int rc;
+
/*
* Relax perms on .text to be RWX, so we can modify them.
*
@@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
PAGE_HYPERVISOR_RWX);
flush_local(FLUSH_TLB_GLOBAL);
- _apply_alternatives(__alt_instructions, __alt_instructions_end,
- alt_done);
+ rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
+ alt_done);
+ if ( rc )
+ panic("Unable to apply alternatives: %d\n", rc);
/*
* Reinstate perms on .text to be RX. This also cleans out the dirty
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index a86eadfaecbd..69555d781ef9 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -24,7 +24,7 @@ struct __packed alt_instr {
extern void add_nops(void *insns, unsigned int len);
/* Similar to alternative_instructions except it can be run with IRQs enabled. */
-extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+extern int apply_alternatives(struct alt_instr *start, struct alt_instr *end);
extern void alternative_instructions(void);
extern void alternative_branches(void);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index f7db4be96e66..6793cb60d1e2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -888,7 +888,15 @@ static int prepare_payload(struct payload *payload,
return -EINVAL;
}
}
- apply_alternatives(start, end);
+
+ rc = apply_alternatives(start, end);
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n",
+ elf->name, rc);
+ return rc;
+ }
+
alt_done:;
#else
printk(XENLOG_ERR LIVEPATCH "%s: We don't support alternative patching\n",
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/5] x86/alternatives: do not BUG during apply
2024-09-26 10:14 ` [PATCH v3 4/5] x86/alternatives: do not BUG during apply Roger Pau Monne
@ 2024-09-26 11:09 ` Andrew Cooper
2024-09-26 11:28 ` Roger Pau Monné
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2024-09-26 11:09 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich, Ross Lagerwall
On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 7824053c9d33..990b7c600932 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
> uint8_t buf[MAX_PATCH_LEN];
> unsigned int total_len = a->orig_len + a->pad_len;
>
> - BUG_ON(a->repl_len > total_len);
> - BUG_ON(total_len > sizeof(buf));
> - BUG_ON(a->cpuid >= NCAPINTS * 32);
> + if ( a->repl_len > total_len )
> + {
> + printk(XENLOG_ERR
> + "alt for %ps, replacement size %#x larger than origin %#x\n",
"Alt" I think. It's both an abbreviation, and the start of the sentence
here.
Can fix on commit.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/5] x86/alternatives: do not BUG during apply
2024-09-26 11:09 ` Andrew Cooper
@ 2024-09-26 11:28 ` Roger Pau Monné
0 siblings, 0 replies; 16+ messages in thread
From: Roger Pau Monné @ 2024-09-26 11:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Jan Beulich, Ross Lagerwall
On Thu, Sep 26, 2024 at 12:09:05PM +0100, Andrew Cooper wrote:
> On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 7824053c9d33..990b7c600932 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
> > uint8_t buf[MAX_PATCH_LEN];
> > unsigned int total_len = a->orig_len + a->pad_len;
> >
> > - BUG_ON(a->repl_len > total_len);
> > - BUG_ON(total_len > sizeof(buf));
> > - BUG_ON(a->cpuid >= NCAPINTS * 32);
> > + if ( a->repl_len > total_len )
> > + {
> > + printk(XENLOG_ERR
> > + "alt for %ps, replacement size %#x larger than origin %#x\n",
>
> "Alt" I think. It's both an abbreviation, and the start of the sentence
> here.
I'm always unsure whether to use uppercase at the beginning of log
messages, because those are preceded by the (XEN) prefix.
> Can fix on commit.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 5/5] x86/alternative: build time check feature is in range
2024-09-26 10:14 [PATCH v3 0/5] xen/livepatch: improvements to loading Roger Pau Monne
` (3 preceding siblings ...)
2024-09-26 10:14 ` [PATCH v3 4/5] x86/alternatives: do not BUG during apply Roger Pau Monne
@ 2024-09-26 10:14 ` Roger Pau Monne
4 siblings, 0 replies; 16+ messages in thread
From: Roger Pau Monne @ 2024-09-26 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
Ensure at build time the feature(s) used for the alternative blocks are in
range of the featureset.
No functional change intended, as all current usages are correct.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
- s/__stringify/STR/.
Changes since v1:
- New in this version.
---
xen/arch/x86/include/asm/alternative-asm.h | 3 +++
xen/arch/x86/include/asm/alternative.h | 4 ++++
2 files changed, 7 insertions(+)
diff --git a/xen/arch/x86/include/asm/alternative-asm.h b/xen/arch/x86/include/asm/alternative-asm.h
index 4092f5ba70a6..83e8594f0eaf 100644
--- a/xen/arch/x86/include/asm/alternative-asm.h
+++ b/xen/arch/x86/include/asm/alternative-asm.h
@@ -12,6 +12,9 @@
* instruction. See apply_alternatives().
*/
.macro altinstruction_entry orig, repl, feature, orig_len, repl_len, pad_len
+ .if \feature >= NCAPINTS * 32
+ .error "alternative feature outside of featureset range"
+ .endif
.long \orig - .
.long \repl - .
.word \feature
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index 69555d781ef9..38472fb58e2d 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -7,6 +7,7 @@
#include <xen/lib.h>
#include <xen/stringify.h>
#include <asm/asm-macros.h>
+#include <asm/cpufeatureset.h>
struct __packed alt_instr {
int32_t orig_offset; /* original instruction */
@@ -59,6 +60,9 @@ extern void alternative_branches(void);
alt_repl_len(n2)) "-" alt_orig_len)
#define ALTINSTR_ENTRY(feature, num) \
+ " .if " STR(feature) " >= " STR(NCAPINTS * 32) "\n" \
+ " .error \"alternative feature outside of featureset range\"\n" \
+ " .endif\n" \
" .long .LXEN%=_orig_s - .\n" /* label */ \
" .long " alt_repl_s(num)" - .\n" /* new instruction */ \
" .word " __stringify(feature) "\n" /* feature bit */ \
--
2.46.0
^ permalink raw reply related [flat|nested] 16+ messages in thread