Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote:
> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -70,6 +70,9 @@ struct payload {
> >      unsigned int nsyms;                  /* Nr of entries in .strtab and symbols. */
> >      struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
> >      struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
> > +    void **bss;                          /* .bss's of the payload. */
> > +    size_t *bss_size;                    /* and their sizes. */
> 
> Is size_t wide enough in the extreme case? Perhaps yes, because I
> don't think we'll ever load 64-bit ELF on a 32-bit platform.

Nonethless having a huge .bss is a kind of extreme? Perhaps we should
have an seperate patch that checks the SHT_NOBITS and disallows .bss's
bigger than say 2MB?

I am using 2MB as that is the limit of the livepatch binaries right
now (see verify_payload:

                                                                             
 127     if ( upload->size > MB(2) )                                                 
 128         return -EINVAL;    


> 
> > +    size_t n_bss;                        /* Size of the array. */
> 
> As opposed to that, I think this one could be unsigned int (or else
> you end up with inconsistencies in {move,apply}_payload()).

/me nods. Changed to unsitned int.
> 
> > @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >                          elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> >              }
> >              else
> > -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > +            {
> > +                payload->bss[n_bss] = elf->sec[i].load_addr;
> > +                payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size;
> > +            }
> >          }
> >      }
> > +    ASSERT(n_bss == payload->n_bss);
> >  
> >   out:
> >      xfree(offset);
> >  
> >      return rc;
> > +
> > + out_mem:
> > +    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n",
> > +            elf->name);
> > +    rc = -ENOMEM;
> > +    goto out;
> 
> You leak any of the three buffers here which you managed to
> successfully allocate.

I added a call to 'free_payload_data(payload)' there to make it a direct call to it.
It is not needed per say as the caller unconditionally calls free_payload_data() if
the return of any of the functions is non-zero. But in case things get moved around
and that assumption goes away - having a call to free_payload_data makes sense
in the function (plus it looks much more nicer to free/alloc in the same function).

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel