Re: [Xen-devel] [PATCH v1 1/3] xen/livepatch: Tighten alignment checks.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 07/11/17 6:53 PM >>>
>--- a/xen/common/livepatch.c
>+++ b/xen/common/livepatch.c
>@@ -406,6 +406,15 @@ static int move_payload(struct payload *payload, struct 
>livepatch_elf *elf)
             >ASSERT(offset[i] != UINT_MAX);
 >
             >elf->sec[i].load_addr = buf + offset[i];
>+            if ( elf->sec[i].sec->sh_addralign > 1 &&

I think ruling out just zero here would be sufficient and look more "natural".

>+                 ((Elf_Addr)elf->sec[i].load_addr % 
>elf->sec[i].sec->sh_addralign) )

The cast here mkes me wonder what it is that you're checking, or
whether the check isn't being done later than it should be done: I'd
expect all such to happen on the original section header, where no
pointer types exist (and hence such a cast shouldn't be needed).

And then - what about the alignment not being a power of 2? Is that
well defined?

>--- a/xen/common/livepatch_elf.c
>+++ b/xen/common/livepatch_elf.c
>@@ -86,6 +86,13 @@ static int elf_resolve_sections(struct livepatch_elf *elf, 
>const void *data)
                     >delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past 
end");
             >return -EINVAL;
         >}
>+        else if ( sec[i].sec->sh_addralign > 1 &&
>+                  sec[i].sec->sh_addr % sec[i].sec->sh_addralign )

Ah, here it is - why the second check further up then?

>+        {
>+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] size 
>(%"PRIuElfWord") is not aligned properly (%"PRIuElfWord")\n",
>+                    elf->name, i, sec[i].sec->sh_size, 
>sec[i].sec->sh_addralign);

What does section size (being logged here) have to do with the check that
failed? I also question the use of decimal values here - generally I find sizes,
offsets, and alignments easier to deal with when they are being presented
as hex numbers.

>--- a/xen/include/xen/elfstructs.h
>+++ b/xen/include/xen/elfstructs.h
>@@ -555,6 +555,7 @@ typedef struct {
 >
 >#if defined(ELFSIZE) && (ELFSIZE == 32)
 >#define PRIxElfAddr   "08x"
>+#define PRIuElfWord   "08u"
 
And leading zeros would cause even more confusion, as they would suggest
(at least to C programmers) the number to be octal.

Jan


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