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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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.

> +    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()).

> @@ -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.

Jan


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