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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 26, 2024 at 09:24:56AM +0100, Andrew Cooper wrote:
> On 26/09/2024 9:22 am, Roger Pau Monné wrote:
> > 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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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 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.
> > So this is how it ends up looking,  there's a bit of churn due to
> > having to drop const on some function parameters.
> 
> Looks fine to me.
> 
> I'd be tempted to name the final field 'addr' because for most of its
> life it is the load address.

I've changed to `addr`.  I however feel it's kind of redundant to name
a pointer field `addr`, as by the type itself (being a pointer) it's
clear it's an address.

> For the comment on the field, I'd say "this is first a temporary buffer,
> then later the load address".

Adjusted.

Thanks, Roger.