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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew