| Message ID | 20240920093656.48879-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/livepatch: improvements to loading | expand |
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
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.
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;
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(-)