kmvtool: invalid embedded ELF binaries' size

kmvtool: invalid embedded ELF binaries' size

[G. Campana]

Hi,

A user of the NoFear project reported a bug (
https://github.com/cappsule/nofear/issues/6 ) suggesting that kvmtool is
broken if compiled with a specific gcc version. This issue can be
reproduced with gcc version 6.2.1 20161124 (Debian 6.2.1-5), but not
with gcc version 6.1.1 20160802 (Debian 6.1.1-11).

The function extract_file() writes embedded ELF files to the filesystem:

    static int extract_file(const char *guestfs_name, const char *filename,
			    const void *data, const void *_size)
        ...
	    ret = xwrite(fd, data, (size_t)_size);
        ...
    }

    extern char _binary_guest_init_start;
    extern char _binary_guest_init_size;
    extern char _binary_guest_pre_init_start;
    extern char _binary_guest_pre_init_size;

I didn't manage to find how _binary_guest_init_size and
_binary_guest_pre_init_size are created during the link step, but a
quick debug session shows that they're 4 bytes wide:

    (gdb) disass kvm_setup_guest_init
    Dump of assembler code for function kvm_setup_guest_init:
    => 0x00005599b846dc60 <+0>:     push   rbx
       0x00005599b846dc61 <+1>:     lea    rcx,[rip+0xffffffffffff76a8]
      # 0x5599b8465310
       0x00005599b846dc68 <+8>:     lea    rdx,[rip+0x22a809]        #
0x5599b8698478
       0x00005599b846dc6f <+15>:    lea    rsi,[rip+0x18a2c]        #
0x5599b84866a2
       0x00005599b846dc76 <+22>:    mov    rbx,rdi
       0x00005599b846dc79 <+25>:    call   0x5599b846d9e0 <extract_file>
       ...
    (gdb) x/xw 0x5599b8465310
    0x5599b8465310: 0x000004c2
    (gdb) x/xg 0x5599b8465310
    0x5599b8465310: 0x00000012000004c2

Casting _size to size_t in extract_file() is wrong because _size is 4
bytes wide. On x64, xwrite thus fails when the 4 bytes following
_binary_guest_init_size/_binary_guest_pre_init_size are different than 0.

I think that casting _size to unsigned int should fix this issue, but I
would appreciate if someone can explain how _binary_guest_init_size and
_binary_guest_pre_init_size are produced by the linker.

Thanks

kvmtool: invalid embedded ELF binaries' size

[G. Campana]

Hi,

I just noticed that a typo is present in the original subject (kvmtool
is misspelled.) I assume that some of you filter messages according to
their subject, hence this new mail.

Sorry for the spam.

-------- Forwarded Message --------
Subject: kmvtool: invalid embedded ELF binaries' size
Date: Mon, 2 Jan 2017 15:04:50 +0100
To: kvm@vger.kernel.org

Hi,

A user of the NoFear project reported a bug (
https://github.com/cappsule/nofear/issues/6 ) suggesting that kvmtool is
broken if compiled with a specific gcc version. This issue can be
reproduced with gcc version 6.2.1 20161124 (Debian 6.2.1-5), but not
with gcc version 6.1.1 20160802 (Debian 6.1.1-11).

The function extract_file() writes embedded ELF files to the filesystem:

    static int extract_file(const char *guestfs_name, const char *filename,
			    const void *data, const void *_size)
        ...
	    ret = xwrite(fd, data, (size_t)_size);
        ...
    }

    extern char _binary_guest_init_start;
    extern char _binary_guest_init_size;
    extern char _binary_guest_pre_init_start;
    extern char _binary_guest_pre_init_size;

I didn't manage to find how _binary_guest_init_size and
_binary_guest_pre_init_size are created during the link step, but a
quick debug session shows that they're 4 bytes wide:

    (gdb) disass kvm_setup_guest_init
    Dump of assembler code for function kvm_setup_guest_init:
    => 0x00005599b846dc60 <+0>:     push   rbx
       0x00005599b846dc61 <+1>:     lea    rcx,[rip+0xffffffffffff76a8]
      # 0x5599b8465310
       0x00005599b846dc68 <+8>:     lea    rdx,[rip+0x22a809]        #
0x5599b8698478
       0x00005599b846dc6f <+15>:    lea    rsi,[rip+0x18a2c]        #
0x5599b84866a2
       0x00005599b846dc76 <+22>:    mov    rbx,rdi
       0x00005599b846dc79 <+25>:    call   0x5599b846d9e0 <extract_file>
       ...
    (gdb) x/xw 0x5599b8465310
    0x5599b8465310: 0x000004c2
    (gdb) x/xg 0x5599b8465310
    0x5599b8465310: 0x00000012000004c2

Casting _size to size_t in extract_file() is wrong because _size is 4
bytes wide. On x64, xwrite thus fails when the 4 bytes following
_binary_guest_init_size/_binary_guest_pre_init_size are different than 0.

I think that casting _size to unsigned int should fix this issue, but I
would appreciate if someone can explain how _binary_guest_init_size and
_binary_guest_pre_init_size are produced by the linker.

Thanks

Re: kvmtool: invalid embedded ELF binaries' size

[G. Campana]

Hello,

My previous analysis of this issue is wrong. Here's the output of nm:

    $ nm guest/guest_pre_init.o
    0000000000000310 D _binary_guest_pre_init_end
    0000000000000310 A _binary_guest_pre_init_size
    0000000000000000 D _binary_guest_pre_init_start

    $ nm lkvm | grep guest_pre_init
    0000000000233788 D _binary_guest_pre_init_end
    0000000000000310 A _binary_guest_pre_init_size
    0000000000233478 D _binary_guest_pre_init_start

According to nm manpage:

    "A" The symbol's value is absolute, and will not be changed by
further linking.

But debugging lkvm with gdb clearly shows that the value of
_binary_guest_pre_init_size changes and is relocated:

    $ rm -rf ~/.lkvm/blah/
    $ gdb -q lkvm
    gdb$ x/2i kvm_setup_guest_init
       0x8c60 <kvm_setup_guest_init>:       push   rbx
       0x8c61 <kvm_setup_guest_init+1>:     lea
rcx,[rip+0xffffffffffff76a8]        # 0x310

    gdb$ r setup blah
    [code]
    => 0x55555555cc60 <kvm_setup_guest_init>:       push   rbx
       0x55555555cc61 <kvm_setup_guest_init+1>:     lea
rcx,[rip+0xffffffffffff76a8]        # 0x555555554310
    Breakpoint 1, kvm_setup_guest_init (guestfs_name=0x7fffffffe33a
"blah") at builtin-setup.c:156

So I finally think this is an issue in gcc...

On 01/03/2017 03:57 PM, G. Campana wrote:
> Hi,
> 
> I just noticed that a typo is present in the original subject (kvmtool
> is misspelled.) I assume that some of you filter messages according to
> their subject, hence this new mail.
> 
> Sorry for the spam.
> 
> -------- Forwarded Message --------
> Subject: kmvtool: invalid embedded ELF binaries' size
> Date: Mon, 2 Jan 2017 15:04:50 +0100
> To: kvm@vger.kernel.org
> 
> Hi,
> 
> A user of the NoFear project reported a bug (
> https://github.com/cappsule/nofear/issues/6 ) suggesting that kvmtool is
> broken if compiled with a specific gcc version. This issue can be
> reproduced with gcc version 6.2.1 20161124 (Debian 6.2.1-5), but not
> with gcc version 6.1.1 20160802 (Debian 6.1.1-11).
> 
> The function extract_file() writes embedded ELF files to the filesystem:
> 
>     static int extract_file(const char *guestfs_name, const char *filename,
> 			    const void *data, const void *_size)
>         ...
> 	    ret = xwrite(fd, data, (size_t)_size);
>         ...
>     }
> 
>     extern char _binary_guest_init_start;
>     extern char _binary_guest_init_size;
>     extern char _binary_guest_pre_init_start;
>     extern char _binary_guest_pre_init_size;
> 
> I didn't manage to find how _binary_guest_init_size and
> _binary_guest_pre_init_size are created during the link step, but a
> quick debug session shows that they're 4 bytes wide:
> 
>     (gdb) disass kvm_setup_guest_init
>     Dump of assembler code for function kvm_setup_guest_init:
>     => 0x00005599b846dc60 <+0>:     push   rbx
>        0x00005599b846dc61 <+1>:     lea    rcx,[rip+0xffffffffffff76a8]
>       # 0x5599b8465310
>        0x00005599b846dc68 <+8>:     lea    rdx,[rip+0x22a809]        #
> 0x5599b8698478
>        0x00005599b846dc6f <+15>:    lea    rsi,[rip+0x18a2c]        #
> 0x5599b84866a2
>        0x00005599b846dc76 <+22>:    mov    rbx,rdi
>        0x00005599b846dc79 <+25>:    call   0x5599b846d9e0 <extract_file>
>        ...
>     (gdb) x/xw 0x5599b8465310
>     0x5599b8465310: 0x000004c2
>     (gdb) x/xg 0x5599b8465310
>     0x5599b8465310: 0x00000012000004c2
> 
> Casting _size to size_t in extract_file() is wrong because _size is 4
> bytes wide. On x64, xwrite thus fails when the 4 bytes following
> _binary_guest_init_size/_binary_guest_pre_init_size are different than 0.
> 
> I think that casting _size to unsigned int should fix this issue, but I
> would appreciate if someone can explain how _binary_guest_init_size and
> _binary_guest_pre_init_size are produced by the linker.
> 
> Thanks
> 

Re: kvmtool: invalid embedded ELF binaries' size

[G. Campana]

[-- Attachment #1: Type: text/plain, Size: 4929 bytes --]

Hi,

Here's a quick shell script which reproduces the bug with a PIE program:

 $ ./pie.sh
   3: 0000000000000004 0 NOTYPE GLOBAL DEFAULT ABS _binary_bar_txt_size
   9: 0000000000000004 0 NOTYPE GLOBAL DEFAULT ABS _binary_bar_txt_size
  50: 0000000000000004 0 NOTYPE GLOBAL DEFAULT ABS _binary_bar_txt_size
 0x563bd4d39004

Expected value is 0x4 while 0x563bd4d39004 is printed because
_binary_bar_txt_size is relocated. According to man 5 elf,

    SHN_ABS       This value specifies absolute values  for  the
                  corresponding reference.  For example, symbols
                  defined relative  to  section  number  SHN_ABS
                  have  absolute  values and are not affected by
                  relocation.

_binary_bar_txt_size shouldn't be relocated, but I don't know if it
makes sense to use the address of an absolute symbol in a PIE binary.

The patch sent in the following mail fixes this issue.

On 01/04/2017 06:19 PM, G. Campana wrote:
> Hello,
> 
> My previous analysis of this issue is wrong. Here's the output of nm:
> 
>     $ nm guest/guest_pre_init.o
>     0000000000000310 D _binary_guest_pre_init_end
>     0000000000000310 A _binary_guest_pre_init_size
>     0000000000000000 D _binary_guest_pre_init_start
> 
>     $ nm lkvm | grep guest_pre_init
>     0000000000233788 D _binary_guest_pre_init_end
>     0000000000000310 A _binary_guest_pre_init_size
>     0000000000233478 D _binary_guest_pre_init_start
> 
> According to nm manpage:
> 
>     "A" The symbol's value is absolute, and will not be changed by
> further linking.
> 
> But debugging lkvm with gdb clearly shows that the value of
> _binary_guest_pre_init_size changes and is relocated:
> 
>     $ rm -rf ~/.lkvm/blah/
>     $ gdb -q lkvm
>     gdb$ x/2i kvm_setup_guest_init
>        0x8c60 <kvm_setup_guest_init>:       push   rbx
>        0x8c61 <kvm_setup_guest_init+1>:     lea
> rcx,[rip+0xffffffffffff76a8]        # 0x310
> 
>     gdb$ r setup blah
>     [code]
>     => 0x55555555cc60 <kvm_setup_guest_init>:       push   rbx
>        0x55555555cc61 <kvm_setup_guest_init+1>:     lea
> rcx,[rip+0xffffffffffff76a8]        # 0x555555554310
>     Breakpoint 1, kvm_setup_guest_init (guestfs_name=0x7fffffffe33a
> "blah") at builtin-setup.c:156
> 
> So I finally think this is an issue in gcc...
> 
> On 01/03/2017 03:57 PM, G. Campana wrote:
>> Hi,
>>
>> I just noticed that a typo is present in the original subject (kvmtool
>> is misspelled.) I assume that some of you filter messages according to
>> their subject, hence this new mail.
>>
>> Sorry for the spam.
>>
>> -------- Forwarded Message --------
>> Subject: kmvtool: invalid embedded ELF binaries' size
>> Date: Mon, 2 Jan 2017 15:04:50 +0100
>> To: kvm@vger.kernel.org
>>
>> Hi,
>>
>> A user of the NoFear project reported a bug (
>> https://github.com/cappsule/nofear/issues/6 ) suggesting that kvmtool is
>> broken if compiled with a specific gcc version. This issue can be
>> reproduced with gcc version 6.2.1 20161124 (Debian 6.2.1-5), but not
>> with gcc version 6.1.1 20160802 (Debian 6.1.1-11).
>>
>> The function extract_file() writes embedded ELF files to the filesystem:
>>
>>     static int extract_file(const char *guestfs_name, const char *filename,
>> 			    const void *data, const void *_size)
>>         ...
>> 	    ret = xwrite(fd, data, (size_t)_size);
>>         ...
>>     }
>>
>>     extern char _binary_guest_init_start;
>>     extern char _binary_guest_init_size;
>>     extern char _binary_guest_pre_init_start;
>>     extern char _binary_guest_pre_init_size;
>>
>> I didn't manage to find how _binary_guest_init_size and
>> _binary_guest_pre_init_size are created during the link step, but a
>> quick debug session shows that they're 4 bytes wide:
>>
>>     (gdb) disass kvm_setup_guest_init
>>     Dump of assembler code for function kvm_setup_guest_init:
>>     => 0x00005599b846dc60 <+0>:     push   rbx
>>        0x00005599b846dc61 <+1>:     lea    rcx,[rip+0xffffffffffff76a8]
>>       # 0x5599b8465310
>>        0x00005599b846dc68 <+8>:     lea    rdx,[rip+0x22a809]        #
>> 0x5599b8698478
>>        0x00005599b846dc6f <+15>:    lea    rsi,[rip+0x18a2c]        #
>> 0x5599b84866a2
>>        0x00005599b846dc76 <+22>:    mov    rbx,rdi
>>        0x00005599b846dc79 <+25>:    call   0x5599b846d9e0 <extract_file>
>>        ...
>>     (gdb) x/xw 0x5599b8465310
>>     0x5599b8465310: 0x000004c2
>>     (gdb) x/xg 0x5599b8465310
>>     0x5599b8465310: 0x00000012000004c2
>>
>> Casting _size to size_t in extract_file() is wrong because _size is 4
>> bytes wide. On x64, xwrite thus fails when the 4 bytes following
>> _binary_guest_init_size/_binary_guest_pre_init_size are different than 0.
>>
>> I think that casting _size to unsigned int should fix this issue, but I
>> would appreciate if someone can explain how _binary_guest_init_size and
>> _binary_guest_pre_init_size are produced by the linker.
>>
>> Thanks
>>

[-- Attachment #2: pie.sh --]
[-- Type: application/x-shellscript, Size: 394 bytes --]

[PATCH] kvmtool: builtin-setup: fix embedded ELF binaries size

[G. Campana]

lkvm embeds 2 ELF binaries (guest/pre_init and guest/init) which are written to
the disk during the setup of new virtual machines. The binaries size is
gathered thanks to a symbol generated by ld during the link step
(_binary_%s_size). Unfortunately, the code generated by the compiler doesn't
always match our expectation because the symbol is relocated.

This commit computes the binaries size in a more reliable way thanks to the
subtraction of the end and start addresses of the binaries.
---
 builtin-setup.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin-setup.c b/builtin-setup.c
index 8be8d62..7ecaf04 100644
--- a/builtin-setup.c
+++ b/builtin-setup.c
@@ -123,7 +123,7 @@ static const char *guestfs_symlinks[] = {
 
 #ifdef CONFIG_GUEST_INIT
 static int extract_file(const char *guestfs_name, const char *filename,
-			const void *data, const void *_size)
+			const void *data, const size_t size)
 {
 	char path[PATH_MAX];
 	int fd, ret;
@@ -138,7 +138,7 @@ static int extract_file(const char *guestfs_name, const char *filename,
 		die("Fail to setup %s", path);
 	}
 
-	ret = xwrite(fd, data, (size_t)_size);
+	ret = xwrite(fd, data, size);
 	if (ret < 0)
 		die("Fail to setup %s", path);
 	close(fd);
@@ -147,24 +147,27 @@ static int extract_file(const char *guestfs_name, const char *filename,
 }
 
 extern char _binary_guest_init_start;
-extern char _binary_guest_init_size;
+extern char _binary_guest_init_end;
 extern char _binary_guest_pre_init_start;
-extern char _binary_guest_pre_init_size;
+extern char _binary_guest_pre_init_end;
 
 int kvm_setup_guest_init(const char *guestfs_name)
 {
+	ptrdiff_t size;
 	int err;
 
 #ifdef CONFIG_GUEST_PRE_INIT
+	size = &_binary_guest_pre_init_end - &_binary_guest_pre_init_start;
 	err = extract_file(guestfs_name, "virt/pre_init",
 				&_binary_guest_pre_init_start,
-				&_binary_guest_pre_init_size);
+				size);
 	if (err)
 		return err;
 #endif
+	size = &_binary_guest_init_end - &_binary_guest_init_start;
 	err = extract_file(guestfs_name, "virt/init",
 				&_binary_guest_init_start,
-				&_binary_guest_init_size);
+				size);
 	return err;
 }
 #else
-- 
2.7.4