Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Sep 25, 2024 at 07:28:42PM +0100, Andrew Cooper wrote:
> On 25/09/2024 11:00 am, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
> >> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> >>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
> >>> load_addr field that points to the stable loaded section data.  Zero the data
> >>> field once load_addr is set, as it would otherwise become a dangling pointer
> >>> once the load buffer is freed.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Changes since v1:
> >>>  - New in this version.
> >>> ---
> >>>  xen/common/livepatch.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> >>> index df41dcce970a..87b3db03e26d 100644
> >>> --- a/xen/common/livepatch.c
> >>> +++ b/xen/common/livepatch.c
> >>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >>>              }
> >>>              else
> >>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> >>> +
> >>> +            /* Avoid leaking pointers to temporary load buffers. */
> >>> +            elf->sec[i].data = NULL;
> >>>          }
> >>>      }
> >>>  
> >> Where is the data allocated and freed?
> >>
> >> I don't see it being freed in this loop, so how is freed subsequently?
> > It's allocated and freed by livepatch_upload(), it's the raw_data
> > buffer that's allocated in the context of that function.
> 
> Well, this is a mess isn't it.
> 
> Ok, so livepatch_upload() makes a temporary buffer to copy everything into.
> 
> In elf_resolve_sections(), we set up sec[i].data pointing into this
> temporary buffer.
> 
> And here, we copy the data from the temporary buffer, into the final
> destination in the Xen .text/data/rodata region.
> 
> So yes, this does end up being a dangling pointer, and clobbering it is
> good.
> 
> 
> But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
> it be better to drop this second pointer and just have move_payload()
> update it here?

I didn't specially like the usage of either load_addr or data in patch
4.

I see, so move_payload() would replace ->data with the relocated
pointer.  One slightly nice thing about the current arrangement is
that data is const, with that change it should become non-const, as we
do modify the contents of load_addr in some case (like sorting the
exception table).  I don't think the constness of data was that
useful, as it's just the temporary buffer.

> I can't see anything good which can come from having two addresses, nor
> a reason why we'd need both.
> 
> Then again, if this is too hard to arrange, it probably can be left as
> an exercise to a future developer.

I can see if it's mostly a trivial change.

Thanks, Roger.