Re: [PATCH v5 23/28] xsplice: Stacking build-id dependency checking.

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

>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote:
> @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against.
>  The old code allows much more flexibility and an additional guard,
>  but is more complex to implement.
>  
> +The second option which requires an build-id of the hypervisor
> +is implemented in the Xen Project hypervisor.
> +
> +Specifically each payload has two build-id ELF notes:
> + * The build-id of the payload itself (generated via --build-id).
> + * The build-id of the payload it depends on (extracted from the
> +   the previous payload or hypervisor during build time).
> +
> +This means that the very first payload depends on the hypervisor
> +build-id.

So this is mean to be a singly linked chain, not something with
branches and alike, allowing independent patches to be applied
solely based on the base build ID? Is such a restriction not going
to get in the way rather sooner than later?

> +# Not Yet Done
> +
> +This is for further development of xSplice.
> +
> +## Goals
> +
> +The implementation must also have a mechanism for:
> +
> + * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload.
> + * Be able to patch .rodata, .bss, and .data sections.
> + * Further safety checks (blacklist of which functions cannot be patched, check
> +   the stack, make sure the payload is built with same compiler as hypervisor,
> +   and NMI/MCE handlers and do_nmi for right now - until an safe solution is found).
> + * NOP out the code sequence if `new_size` is zero.
> + * Deal with other relocation types:  R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file.

Does this belong here? Doesn't this duplicate something I saw earlier?

> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -70,10 +70,29 @@ const char *xen_deny(void)
>  /* Defined in linker script. */
>  extern const Elf_Note __note_gnu_build_id_start[], 
> __note_gnu_build_id_end[];
>  
> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len)
> +{
> +    /* Check if we really have a build-id. */
> +    if ( NT_GNU_BUILD_ID != n->type )
> +        return -ENODATA;
> +
> +    /* Sanity check, name should be "GNU" for ld-generated build-id. */
> +    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
> +        return -ENODATA;

For the embedded notes this suffices as verification, but I question
this being enough for a patch module: No part of the note should
exceed the containing section. And maybe there are other things.

>  #else
>  
> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len)
> +{
> +    return -ENODATA;
> +}

What case is this needed for, considering that only xSplice code
should be calling it, and that code depends on build ID availability.

> +static int build_id_dep(struct payload *payload, bool_t ignore)
> +{
> +    const void *id = NULL;
> +    unsigned int len = 0;
> +    int rc;
> +    const char *name = "hypervisor";
> +
> +    ASSERT(payload->dep.len && payload->dep.p);
> +
> +    /* First time user is against hypervisor. */
> +    if ( ignore || list_empty(&applied_list) )

"ignore" is perhaps not the most descriptive name, as you aren't
ignoring anything here. Maybe "internal"? And then maybe have
the caller pass the argument using list_empty(&applied_list)
instead of you checking it here?

> --- a/xen/include/xen/version.h
> +++ b/xen/include/xen/version.h
> @@ -17,4 +17,7 @@ const char *xen_deny(void);
>  #include <xen/types.h>
>  int xen_build_id(const void **p, unsigned int *len);
>  
> +#include <xen/elfstructs.h>
> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len);

The #include is misplaced again, and I'm rather hesitant to see
version.h gain this dependency. Couldn't this go into xen/elf.h?

> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -40,6 +40,11 @@ struct xsplice_symbol {
>      bool_t new_symbol;
>  };
>  
> +struct xsplice_build_id {
> +   const void *p;
> +   unsigned int len;
> +};

This isn't being used outside of xsplice.c, so please define the
structure there.

Jan

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