| Message ID | 20170711165313.26497-2-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index ca36161..5d53096 100644 --- 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 && + ((Elf_Addr)elf->sec[i].load_addr % elf->sec[i].sec->sh_addralign) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s @ %p is not aligned (%"PRIuElfWord")\n", + elf->name, elf->sec[i].name, elf->sec[i].load_addr, + elf->sec[i].sec->sh_addralign); + rc = -EINVAL; + goto out; + } /* Don't copy NOBITS - such as BSS. */ if ( elf->sec[i].sec->sh_type != SHT_NOBITS ) diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index b69e271..852e9c4 100644 --- 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 ) + { + 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); + return -EINVAL; + } else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) && sec[i].sec->sh_type == SHT_NOBITS && sec[i].sec->sh_size > LIVEPATCH_MAX_SIZE ) diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h index 950e149..edc8862 100644 --- 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" #define Elf_Ehdr Elf32_Ehdr #define Elf_Phdr Elf32_Phdr @@ -582,6 +583,7 @@ typedef struct { #define AuxInfo Aux32Info #elif defined(ELFSIZE) && (ELFSIZE == 64) #define PRIxElfAddr PRIx64 +#define PRIuElfWord PRIu64 #define Elf_Ehdr Elf64_Ehdr #define Elf_Phdr Elf64_Phdr
The ELF specification mentions nothing about the sh_size being modulo the sh_addralign. Only that sh_addr MUST be aligned on sh_addralign if sh_addralign is not zero or one. We on loading did not take this in-to account so this patch adds two checks: One on the ELF file itself as it is being parsed, and then when we copy the payload contents in memory - and check the aligmnent needs there. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/common/livepatch.c | 9 +++++++++ xen/common/livepatch_elf.c | 7 +++++++ xen/include/xen/elfstructs.h | 2 ++ 3 files changed, 18 insertions(+)