| Message ID | 20220317110854.39050-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | livepatch: fix handling of (some) relocations | expand |
On 17/03/2022 11:08, Roger Pau Monne wrote: > A side effect of ignoring such sections is that symbols belonging to > them won't be resolved, and that could make relocations belonging to > other sections that reference those symbols fail. > > For example it's likely to have an empty .altinstr_replacement with > symbols pointing to it, and marking the section as ignored will > prevent the symbols from being resolved, which in turn will cause any > relocations against them to fail. I agree this is a bug in livepatch handling, but it's also an error in the generated livepatch. We should not have relocations to an empty altinstr_replacement section in the first place. This will probably be from the lfences, where the replacement in a nop and takes no space. I think I know how to fix this case. ~Andrew
On 17.03.2022 15:00, Andrew Cooper wrote: > On 17/03/2022 11:08, Roger Pau Monne wrote: >> A side effect of ignoring such sections is that symbols belonging to >> them won't be resolved, and that could make relocations belonging to >> other sections that reference those symbols fail. >> >> For example it's likely to have an empty .altinstr_replacement with >> symbols pointing to it, and marking the section as ignored will >> prevent the symbols from being resolved, which in turn will cause any >> relocations against them to fail. > > I agree this is a bug in livepatch handling, but it's also an error in > the generated livepatch. We should not have relocations to an empty > altinstr_replacement section in the first place. I think having such relocations is quite natural, but ... > This will probably be from the lfences, where the replacement in a nop > and takes no space. I think I know how to fix this case. ... of course it's possible to eliminate them. Whether that's worthwhile to add logic for I'm not sure. Jan
On Thu, Mar 17, 2022 at 02:00:19PM +0000, Andrew Cooper wrote: > On 17/03/2022 11:08, Roger Pau Monne wrote: > > A side effect of ignoring such sections is that symbols belonging to > > them won't be resolved, and that could make relocations belonging to > > other sections that reference those symbols fail. > > > > For example it's likely to have an empty .altinstr_replacement with > > symbols pointing to it, and marking the section as ignored will > > prevent the symbols from being resolved, which in turn will cause any > > relocations against them to fail. > > I agree this is a bug in livepatch handling, but it's also an error in > the generated livepatch. We should not have relocations to an empty > altinstr_replacement section in the first place. Well, the relocation destination is in the .altinstructions section (which is not empty). It happens however to reference a symbol that points to the .altinstr_replacement section that's empty. We could likely avoid generating the altinstr_replacement section in the first place, but I think it's more robust to handle those properly in the elf loading code. > This will probably be from the lfences, where the replacement in a nop > and takes no space. I think I know how to fix this case. Indeed, that's my understanding. Thanks, Roger.
On 17/03/2022 14:16, Roger Pau Monné wrote: > On Thu, Mar 17, 2022 at 02:00:19PM +0000, Andrew Cooper wrote: >> On 17/03/2022 11:08, Roger Pau Monne wrote: >>> A side effect of ignoring such sections is that symbols belonging to >>> them won't be resolved, and that could make relocations belonging to >>> other sections that reference those symbols fail. >>> >>> For example it's likely to have an empty .altinstr_replacement with >>> symbols pointing to it, and marking the section as ignored will >>> prevent the symbols from being resolved, which in turn will cause any >>> relocations against them to fail. >> I agree this is a bug in livepatch handling, but it's also an error in >> the generated livepatch. We should not have relocations to an empty >> altinstr_replacement section in the first place. > Well, the relocation destination is in the .altinstructions section > (which is not empty). It happens however to reference a symbol that > points to the .altinstr_replacement section that's empty. > > We could likely avoid generating the altinstr_replacement section in > the first place, but I think it's more robust to handle those properly > in the elf loading code. Actually, it turns out it's distinctly non-trivial to omit these references. We need to put the replacement somewhere, so we can subtract the start from the end, and figure out if it is 0. ~Andrew
Ping? There was some discussion on whether we need to handle such empty sections, but I think we settled that it's necessary. Thanks, Roger. On Thu, Mar 17, 2022 at 12:08:53PM +0100, Roger Pau Monne wrote: > A side effect of ignoring such sections is that symbols belonging to > them won't be resolved, and that could make relocations belonging to > other sections that reference those symbols fail. > > For example it's likely to have an empty .altinstr_replacement with > symbols pointing to it, and marking the section as ignored will > prevent the symbols from being resolved, which in turn will cause any > relocations against them to fail. > > In order to solve this do not ignore sections with 0 size, only ignore > sections that don't have the SHF_ALLOC flag set. > > Special case such empty sections in move_payload so they are not taken > into account in order to decide whether a livepatch can be safely > re-applied after a revert. > > Fixes: 98b728a7b2 ('livepatch: Disallow applying after an revert') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/common/livepatch.c | 16 +++++++++++----- > xen/include/xen/livepatch_elf.h | 2 +- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index be2cf75c2d..abc1cae136 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -300,9 +300,6 @@ 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 ( livepatch_elf_ignore_section(elf->sec[i].sec) ) > offset[i] = UINT_MAX; > @@ -361,8 +358,17 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) > else if ( elf->sec[i].sec->sh_flags & SHF_WRITE ) > { > buf = rw_buf; > - rw_buf_sec = i; > - rw_buf_cnt++; > + if ( elf->sec[i].sec->sh_size ) > + { > + /* > + * Special handling of RW empty regions: do not account for > + * them in order to decide whether a patch can safely be > + * re-applied, but assign them a load address so symbol > + * resolution and relocations work. > + */ > + rw_buf_sec = i; > + rw_buf_cnt++; > + } > } > else > buf = ro_buf; > diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h > index 9ad499ee8b..5b1ec469da 100644 > --- a/xen/include/xen/livepatch_elf.h > +++ b/xen/include/xen/livepatch_elf.h > @@ -48,7 +48,7 @@ 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; > + return !(sec->sh_flags & SHF_ALLOC); > } > #endif /* __XEN_LIVEPATCH_ELF_H__ */ > > -- > 2.34.1 >
> From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Thursday, March 17, 2022 11:08 AM > To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com> > Subject: [PATCH 1/2] livepatch: do not ignore sections with 0 size > > A side effect of ignoring such sections is that symbols belonging to > them won't be resolved, and that could make relocations belonging to > other sections that reference those symbols fail. > > For example it's likely to have an empty .altinstr_replacement with > symbols pointing to it, and marking the section as ignored will > prevent the symbols from being resolved, which in turn will cause any > relocations against them to fail. > > In order to solve this do not ignore sections with 0 size, only ignore > sections that don't have the SHF_ALLOC flag set. > > Special case such empty sections in move_payload so they are not taken > into account in order to decide whether a livepatch can be safely > re-applied after a revert. > > Fixes: 98b728a7b2 ('livepatch: Disallow applying after an revert') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index be2cf75c2d..abc1cae136 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -300,9 +300,6 @@ 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 ( livepatch_elf_ignore_section(elf->sec[i].sec) ) offset[i] = UINT_MAX; @@ -361,8 +358,17 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) else if ( elf->sec[i].sec->sh_flags & SHF_WRITE ) { buf = rw_buf; - rw_buf_sec = i; - rw_buf_cnt++; + if ( elf->sec[i].sec->sh_size ) + { + /* + * Special handling of RW empty regions: do not account for + * them in order to decide whether a patch can safely be + * re-applied, but assign them a load address so symbol + * resolution and relocations work. + */ + rw_buf_sec = i; + rw_buf_cnt++; + } } else buf = ro_buf; diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h index 9ad499ee8b..5b1ec469da 100644 --- a/xen/include/xen/livepatch_elf.h +++ b/xen/include/xen/livepatch_elf.h @@ -48,7 +48,7 @@ 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; + return !(sec->sh_flags & SHF_ALLOC); } #endif /* __XEN_LIVEPATCH_ELF_H__ */
A side effect of ignoring such sections is that symbols belonging to them won't be resolved, and that could make relocations belonging to other sections that reference those symbols fail. For example it's likely to have an empty .altinstr_replacement with symbols pointing to it, and marking the section as ignored will prevent the symbols from being resolved, which in turn will cause any relocations against them to fail. In order to solve this do not ignore sections with 0 size, only ignore sections that don't have the SHF_ALLOC flag set. Special case such empty sections in move_payload so they are not taken into account in order to decide whether a livepatch can be safely re-applied after a revert. Fixes: 98b728a7b2 ('livepatch: Disallow applying after an revert') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/livepatch.c | 16 +++++++++++----- xen/include/xen/livepatch_elf.h | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-)