[v2,1/5] livepatch: Tighten alignment checks.

Commit Message

Konrad Rzeszutek Wilk July 26, 2017, 7:47 p.m. UTC

From: Konrad Rzeszutek Wilk <[e-mail]>

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
a check on the ELF file as it is being parsed.

Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
---
v1: Initial patch
v2: Drop the check when loading it in memory
    Add check for alignment being anything but power of two (ignoring 0, and 1)
    Change dprintk to include hex values and print addr not size.
---
 xen/common/livepatch_elf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Jan Beulich July 31, 2017, 1:46 p.m. UTC | #1

>>> Konrad Rzeszutek Wilk <[e-mail]> 07/26/17 9:50 PM >>>
>--- a/xen/common/livepatch_elf.c
>+++ b/xen/common/livepatch_elf.c
>@@ -86,6 +86,19 @@ 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 &&

As said before, to me this check looks confusing. I'd recommend to only check
for the field to be non-zero.

>+                  sec[i].sec->sh_addr % sec[i].sec->sh_addralign )
>+        {
>+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] addr (%#"PRIxElfAddr") is not aligned properly (%#"PRIxElfAddr")\n",
>+                    elf->name, i, sec[i].sec->sh_addr, sec[i].sec->sh_addralign);
>+            return -EINVAL;
>+        }
>+        else if ( sec[i].sec->sh_addralign > 1 && sec[i].sec->sh_addralign % 2 )

What use is this one? Do you perhaps mean to check that the alignment is
a power of 2? In that case a single check of sh_addralign & (sh_addralign - 1)
against zero would be what you want.

Jan

Patch

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index b69e2718dd..4dc1b68871 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,6 +86,19 @@  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] addr (%#"PRIxElfAddr") is not aligned properly (%#"PRIxElfAddr")\n",
+                    elf->name, i, sec[i].sec->sh_addr, sec[i].sec->sh_addralign);
+            return -EINVAL;
+        }
+        else if ( sec[i].sec->sh_addralign > 1 && sec[i].sec->sh_addralign % 2 )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] alignment (%#"PRIxElfAddr") is not supported\n",
+                    elf->name, i, sec[i].sec->sh_addralign);
+            return -EOPNOTSUPP;
+        }
         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 )