[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer
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 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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |