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>
---
xen/common/livepatch.c | 106 ++++++++++++++++++-----------------------
1 file changed, 46 insertions(+), 60 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d93a556bcda2..cea47ffe4c84 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
nhooks = __sec->sec->sh_size / sizeof(*hook); \
} while (0)
+static int fetch_buildid(const struct livepatch_elf_sec *sec,
+ struct livepatch_build_id *id)
+{
+ const Elf_Note *n = sec->load_addr;
+ int rc;
+
+ ASSERT(sec);
+
+ 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 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 )
@@ -673,8 +695,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 )
@@ -717,69 +737,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->load_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 = fetch_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->load_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->load_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 = fetch_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 = fetch_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
On 20.09.2024 11:36, Roger Pau Monne 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>
> ---
> xen/common/livepatch.c | 106 ++++++++++++++++++-----------------------
> 1 file changed, 46 insertions(+), 60 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index d93a556bcda2..cea47ffe4c84 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
> nhooks = __sec->sec->sh_size / sizeof(*hook); \
> } while (0)
>
> +static int fetch_buildid(const struct livepatch_elf_sec *sec,
> + struct livepatch_build_id *id)
> +{
> + const Elf_Note *n = sec->load_addr;
> + int rc;
> +
> + ASSERT(sec);
> +
> + if ( sec->sec->sh_size <= sizeof(*n) )
> + return -EINVAL;
Oh, after my reply to Andrew's reply, now looking at the actual change -
is it perhaps ASSERT(sec->sec) that was meant?
Jan
On Mon, Sep 23, 2024 at 01:01:57PM +0200, Jan Beulich wrote:
> On 20.09.2024 11:36, Roger Pau Monne 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>
> > ---
> > xen/common/livepatch.c | 106 ++++++++++++++++++-----------------------
> > 1 file changed, 46 insertions(+), 60 deletions(-)
> >
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index d93a556bcda2..cea47ffe4c84 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
> > nhooks = __sec->sec->sh_size / sizeof(*hook); \
> > } while (0)
> >
> > +static int fetch_buildid(const struct livepatch_elf_sec *sec,
> > + struct livepatch_build_id *id)
> > +{
> > + const Elf_Note *n = sec->load_addr;
> > + int rc;
> > +
> > + ASSERT(sec);
> > +
> > + if ( sec->sec->sh_size <= sizeof(*n) )
> > + return -EINVAL;
>
> Oh, after my reply to Andrew's reply, now looking at the actual change -
> is it perhaps ASSERT(sec->sec) that was meant?
The original check before moving the code was against 'sec', not
'sec->sec'. That's what I intending to retain with the assert.
I can do the `n = sec->load_addr` assign after the assert if that's
better analysis wise.
Thanks, Roger.
On 20/09/2024 11:36 am, Roger Pau Monne wrote:
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index d93a556bcda2..cea47ffe4c84 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
> nhooks = __sec->sec->sh_size / sizeof(*hook); \
> } while (0)
>
> +static int fetch_buildid(const struct livepatch_elf_sec *sec,
> + struct livepatch_build_id *id)
Is this really fetch? I'd describe it as parse, more than fetch.
> +{
> + const Elf_Note *n = sec->load_addr;
> + int rc;
> +
> + ASSERT(sec);
This needs to turn back into a runtime check. Now, if a livepatch is
missing one of the sections, we'll dereference NULL below, rather than
leaving no data in the struct livepatch_build_id.
~Andrew
On Sun, Sep 22, 2024 at 11:19:01AM +0200, Andrew Cooper wrote:
> On 20/09/2024 11:36 am, Roger Pau Monne wrote:
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index d93a556bcda2..cea47ffe4c84 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
> > nhooks = __sec->sec->sh_size / sizeof(*hook); \
> > } while (0)
> >
> > +static int fetch_buildid(const struct livepatch_elf_sec *sec,
> > + struct livepatch_build_id *id)
>
> Is this really fetch? I'd describe it as parse, more than fetch.
I can indeed change the naming. I've used fetch because it 'fetches'
the contents of livepatch_build_id.
> > +{
> > + const Elf_Note *n = sec->load_addr;
> > + int rc;
> > +
> > + ASSERT(sec);
>
> This needs to turn back into a runtime check. Now, if a livepatch is
> missing one of the sections, we'll dereference NULL below, rather than
> leaving no data in the struct livepatch_build_id.
Loading should never get here without those sections being present,
check_special_sections() called earlier will return error if any of
the sections is not present, hence the ASSERT() is fine IMO.
I could do `if ( !sec ) { ASSERT_UNREACHABLE(); return -ENOENT; }`,
but given the code in check_special_sections() that checks the section
presence just ahead it seemed unnecessary convoluted.
Thanks, Roger.
On 23/09/2024 8:46 am, Roger Pau Monné wrote:
> On Sun, Sep 22, 2024 at 11:19:01AM +0200, Andrew Cooper wrote:
>> On 20/09/2024 11:36 am, Roger Pau Monne wrote:
>>> +{
>>> + const Elf_Note *n = sec->load_addr;
>>> + int rc;
>>> +
>>> + ASSERT(sec);
>> This needs to turn back into a runtime check. Now, if a livepatch is
>> missing one of the sections, we'll dereference NULL below, rather than
>> leaving no data in the struct livepatch_build_id.
> Loading should never get here without those sections being present,
> check_special_sections() called earlier will return error if any of
> the sections is not present, hence the ASSERT() is fine IMO.
Ah, in which case, can we please have:
/* Existence of note sections already confirmed in
check_special_sections() */
ASSERT(sec);
Just to show that someone did think about the provenance of the pointer,
and where to look to check.
With this and the rename, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>
On 23.09.2024 11:43, Andrew Cooper wrote:
> On 23/09/2024 8:46 am, Roger Pau Monné wrote:
>> On Sun, Sep 22, 2024 at 11:19:01AM +0200, Andrew Cooper wrote:
>>> On 20/09/2024 11:36 am, Roger Pau Monne wrote:
>>>> +{
>>>> + const Elf_Note *n = sec->load_addr;
>>>> + int rc;
>>>> +
>>>> + ASSERT(sec);
>>> This needs to turn back into a runtime check. Now, if a livepatch is
>>> missing one of the sections, we'll dereference NULL below, rather than
>>> leaving no data in the struct livepatch_build_id.
>> Loading should never get here without those sections being present,
>> check_special_sections() called earlier will return error if any of
>> the sections is not present, hence the ASSERT() is fine IMO.
>
> Ah, in which case, can we please have:
>
> /* Existence of note sections already confirmed in
> check_special_sections() */
> ASSERT(sec);
>
> Just to show that someone did think about the provenance of the pointer,
> and where to look to check.
Yet then sec was de-referenced already ahead of the assertion, which
static checkers may have to say something about.
Jan
© 2016 - 2025 Red Hat, Inc.