Re: [Xen-devel] [PATCH for-4.8] libelf: fix symtab/strtab loading for 32bit domains

[Jan Beulich]

>>> On 05.10.16 at 18:17,  wrote:
> On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote:
>> >>> On 05.10.16 at 17:11,  wrote:
>> > +/*
>> > + * Load the section headers.
>> > + *
>> > + * NB: this _must_ be done one by one, and taking the bitness into 
>> > account,
>> > + * so that the guest can treat this as an array of type 
>> > Elf{32/64}_Shdr.
>> > + */
>> > +shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
>> > +for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
>> > +{
>> > +if ( elf_64bit(elf) )
>> > +shdr = _header.section[i].e64;
>> > +else
>> > +shdr = _header.section[i].e32;
>> > +
>> > +rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i,
>> > +ELF_REALPTR2PTRVAL(shdr), shdr_size, 
>> > shdr_size);
>> 
>> You shouldn't read shdr_size bytes here, but only sizeof() ones.
> 
> Well, shdr_size is just the result of a sizeof.

Oh, I didn't even spot that - it makes things worse: The value gets
retrieved correctly earlier on.

Jan


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

Re: [Xen-devel] [PATCH for-4.8] libelf: fix symtab/strtab loading for 32bit domains

[Roger Pau Monne]

On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote:
> >>> On 05.10.16 at 17:11,  wrote:
> > --- a/xen/common/libelf/libelf-loader.c
> > +++ b/xen/common/libelf/libelf-loader.c
> > @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
> >  } __attribute__((packed)) header;
> >  
> >  ELF_HANDLE_DECL(elf_ehdr) header_handle;
> > -unsigned long shdr_size;
> > +unsigned long shdr_size, ehdr_size;
> >  ELF_HANDLE_DECL(elf_shdr) section_handle;
> > -unsigned int link, rc;
> > +unsigned int link, rc, i;
> >  elf_ptrval header_base;
> >  elf_ptrval elf_header_base;
> >  elf_ptrval symtab_base;
> >  elf_ptrval strtab_base;
> > +void *shdr;
> 
> I'd appreciate if you moved this into the scope where it's needed.
> Also I think it could be const.

Done.
 
> > @@ -394,15 +395,40 @@ do {  
> >  
> >  \
> >  header.size = strtab_base + elf_uval(elf, section_handle, sh_size) -
> >elf_header_base;
> >  
> > -/* Load the headers. */
> > +/* Load the size plus elf header. */
> > +ehdr_size = sizeof(header) - sizeof(header.elf_header.section);
> 
> I think offsetof(typeof(header), elf_header.section) would be the
> safer expression here, removing the dependency on the packed
> attribute.

Right, I was dubious about which one would be better. I've removed the 
packed attribute of elf_header, but the one from header needs to say (so 
that the elf_header is just after the uint32_t). 

> >  rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(),
> > -sizeof(header), sizeof(header));
> > +ehdr_size, ehdr_size);
> >  if ( rc != 0 )
> >  {
> >  elf_mark_broken(elf, "unable to load ELF headers into guest 
> > memory");
> >  return;
> >  }
> >  
> > +/*
> > + * Load the section headers.
> > + *
> > + * NB: this _must_ be done one by one, and taking the bitness into 
> > account,
> > + * so that the guest can treat this as an array of type 
> > Elf{32/64}_Shdr.
> > + */
> > +shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> > +for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
> > +{
> > +if ( elf_64bit(elf) )
> > +shdr = _header.section[i].e64;
> > +else
> > +shdr = _header.section[i].e32;
> > +
> > +rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i,
> > +ELF_REALPTR2PTRVAL(shdr), shdr_size, 
> > shdr_size);
> 
> You shouldn't read shdr_size bytes here, but only sizeof() ones.

Well, shdr_size is just the result of a sizeof.

> Which btw also applies to the earlier memset()ing of the SHN_UNDEF
> header, which it then also looks like gets overwritten by the loading
> done here (irrespective of the change you make).

Hm, I'm not sure I follow. Memset()ing of 
header.elf_header.section[SHN_UNDEF] is required, so that we don't copy 
contents of the stack into guest memory. This memset is done to the local 
data, which is then copied into the guest memory space here.

Roger.

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

Re: [Xen-devel] [PATCH for-4.8] libelf: fix symtab/strtab loading for 32bit domains

[Jan Beulich]

>>> On 05.10.16 at 17:11,  wrote:
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
>  } __attribute__((packed)) header;
>  
>  ELF_HANDLE_DECL(elf_ehdr) header_handle;
> -unsigned long shdr_size;
> +unsigned long shdr_size, ehdr_size;
>  ELF_HANDLE_DECL(elf_shdr) section_handle;
> -unsigned int link, rc;
> +unsigned int link, rc, i;
>  elf_ptrval header_base;
>  elf_ptrval elf_header_base;
>  elf_ptrval symtab_base;
>  elf_ptrval strtab_base;
> +void *shdr;

I'd appreciate if you moved this into the scope where it's needed.
Also I think it could be const.

> @@ -394,15 +395,40 @@ do {   
>  \
>  header.size = strtab_base + elf_uval(elf, section_handle, sh_size) -
>elf_header_base;
>  
> -/* Load the headers. */
> +/* Load the size plus elf header. */
> +ehdr_size = sizeof(header) - sizeof(header.elf_header.section);

I think offsetof(typeof(header), elf_header.section) would be the
safer expression here, removing the dependency on the packed
attribute.

>  rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(),
> -sizeof(header), sizeof(header));
> +ehdr_size, ehdr_size);
>  if ( rc != 0 )
>  {
>  elf_mark_broken(elf, "unable to load ELF headers into guest memory");
>  return;
>  }
>  
> +/*
> + * Load the section headers.
> + *
> + * NB: this _must_ be done one by one, and taking the bitness into 
> account,
> + * so that the guest can treat this as an array of type Elf{32/64}_Shdr.
> + */
> +shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
> +{
> +if ( elf_64bit(elf) )
> +shdr = _header.section[i].e64;
> +else
> +shdr = _header.section[i].e32;
> +
> +rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i,
> +ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size);

You shouldn't read shdr_size bytes here, but only sizeof() ones.

Which btw also applies to the earlier memset()ing of the SHN_UNDEF
header, which it then also looks like gets overwritten by the loading
done here (irrespective of the change you make).

Jan


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

[Xen-devel] [PATCH for-4.8] libelf: fix symtab/strtab loading for 32bit domains

[Roger Pau Monne]

Commit ed04ca introduced a bug in the symtab/strtab loading for 32bit
guests, that corrupted the section headers array due to the padding
introduced by the elf_shdr union.

The Elf section header array on 32bit should be accessible as an array of
Elf32_Shdr elements, and the union with Elf64_Shdr done in elf_shdr was
breaking this due to size differences between Elf32_Shdr and Elf64_Shdr.

Fix this by copying each section header one by one, and using the proper
size depending on the bitness of the guest kernel.

Reported-by: Brian Marcotte 
Signed-off-by: Roger Pau Monné 
---
Cc: Brian Marcotte 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
Should be backported to Xen 4.7 stable branch.
---
 xen/common/libelf/libelf-loader.c | 34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c 
b/xen/common/libelf/libelf-loader.c
index 2626a40..3faff62 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
 } __attribute__((packed)) header;
 
 ELF_HANDLE_DECL(elf_ehdr) header_handle;
-unsigned long shdr_size;
+unsigned long shdr_size, ehdr_size;
 ELF_HANDLE_DECL(elf_shdr) section_handle;
-unsigned int link, rc;
+unsigned int link, rc, i;
 elf_ptrval header_base;
 elf_ptrval elf_header_base;
 elf_ptrval symtab_base;
 elf_ptrval strtab_base;
+void *shdr;
 
 if ( !elf->bsd_symtab_pstart )
 return;
@@ -394,15 +395,40 @@ do {  
  \
 header.size = strtab_base + elf_uval(elf, section_handle, sh_size) -
   elf_header_base;
 
-/* Load the headers. */
+/* Load the size plus elf header. */
+ehdr_size = sizeof(header) - sizeof(header.elf_header.section);
 rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(),
-sizeof(header), sizeof(header));
+ehdr_size, ehdr_size);
 if ( rc != 0 )
 {
 elf_mark_broken(elf, "unable to load ELF headers into guest memory");
 return;
 }
 
+/*
+ * Load the section headers.
+ *
+ * NB: this _must_ be done one by one, and taking the bitness into account,
+ * so that the guest can treat this as an array of type Elf{32/64}_Shdr.
+ */
+shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
+for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
+{
+if ( elf_64bit(elf) )
+shdr = _header.section[i].e64;
+else
+shdr = _header.section[i].e32;
+
+rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i,
+ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size);
+if ( rc != 0 )
+{
+elf_mark_broken(elf,
+"unable to load ELF section header into guest memory");
+return;
+}
+}
+
 /* Remove permissions from elf_memcpy_safe. */
 elf->caller_xdest_base = NULL;
 elf->caller_xdest_size = 0;
-- 
2.7.4 (Apple Git-66)


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