[PATCH] fix attempt to seek outside of the file error when parsing xen ELF notes

[PATCH] fix attempt to seek outside of the file error when parsing xen ELF notes

[Michael Chang]

The kernel bzImage payload length appears to be incorrectly minus by 4 when
trying to grub_file_offset_open for the uncompressed vmlinux image. If gzip
used as compressor it will lead to the file->size read from offset of crc32
instead of the uncompressed image size.

[1]
http://www.onicos.com/staff/iz/formats/gzip.html
---
 grub-core/loader/i386/xen_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/loader/i386/xen_file.c b/grub-core/loader/i386/xen_file.c
index 5836218..37f9ad8 100644
--- a/grub-core/loader/i386/xen_file.c
+++ b/grub-core/loader/i386/xen_file.c
@@ -55,11 +55,11 @@ grub_xen_file (grub_file_t file)
   grub_dprintf ("xen", "found bzimage payload 0x%llx-0x%llx\n",
 		(unsigned long long) (lh.setup_sects + 1) * 512
 		+ lh.payload_offset,
-		(unsigned long long) lh.payload_length - 4);
+		(unsigned long long) lh.payload_length);
 
   off_file = grub_file_offset_open (file, (lh.setup_sects + 1) * 512
 				    + lh.payload_offset,
-				    lh.payload_length - 4);
+				    lh.payload_length);
   if (!off_file)
     goto fail;
 
-- 
1.8.5.6

Re: [PATCH] fix attempt to seek outside of the file error when parsing xen ELF notes

[Andrei Borzenkov]

03.03.2016 10:33, Michael Chang пишет:
> The kernel bzImage payload length appears to be incorrectly minus by 4 when
> trying to grub_file_offset_open for the uncompressed vmlinux image. If gzip
> used as compressor it will lead to the file->size read from offset of crc32
> instead of the uncompressed image size.
> 
> [1]
> http://www.onicos.com/staff/iz/formats/gzip.html
> ---
>  grub-core/loader/i386/xen_file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/loader/i386/xen_file.c b/grub-core/loader/i386/xen_file.c
> index 5836218..37f9ad8 100644
> --- a/grub-core/loader/i386/xen_file.c
> +++ b/grub-core/loader/i386/xen_file.c
> @@ -55,11 +55,11 @@ grub_xen_file (grub_file_t file)
>    grub_dprintf ("xen", "found bzimage payload 0x%llx-0x%llx\n",
>  		(unsigned long long) (lh.setup_sects + 1) * 512
>  		+ lh.payload_offset,
> -		(unsigned long long) lh.payload_length - 4);
> +		(unsigned long long) lh.payload_length);
>  
>    off_file = grub_file_offset_open (file, (lh.setup_sects + 1) * 512
>  				    + lh.payload_offset,
> -				    lh.payload_length - 4);
> +				    lh.payload_length);
>    if (!off_file)
>      goto fail;
>  
> 
Looks strange indeed. I wonder what was original intention, code looked
this way from the very beginning. May be it is required with some other
format?

Re: [PATCH] fix attempt to seek outside of the file error when parsing xen ELF notes

[Michael Chang]

On Thu, Mar 03, 2016 at 09:17:27PM +0300, Andrei Borzenkov wrote:
> 03.03.2016 10:33, Michael Chang пишет:
> > The kernel bzImage payload length appears to be incorrectly minus by 4 when
> > trying to grub_file_offset_open for the uncompressed vmlinux image. If gzip
> > used as compressor it will lead to the file->size read from offset of crc32
> > instead of the uncompressed image size.
> > 
> > [1]
> > http://www.onicos.com/staff/iz/formats/gzip.html
> > ---
> >  grub-core/loader/i386/xen_file.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/grub-core/loader/i386/xen_file.c b/grub-core/loader/i386/xen_file.c
> > index 5836218..37f9ad8 100644
> > --- a/grub-core/loader/i386/xen_file.c
> > +++ b/grub-core/loader/i386/xen_file.c
> > @@ -55,11 +55,11 @@ grub_xen_file (grub_file_t file)
> >    grub_dprintf ("xen", "found bzimage payload 0x%llx-0x%llx\n",
> >  		(unsigned long long) (lh.setup_sects + 1) * 512
> >  		+ lh.payload_offset,
> > -		(unsigned long long) lh.payload_length - 4);
> > +		(unsigned long long) lh.payload_length);
> >  
> >    off_file = grub_file_offset_open (file, (lh.setup_sects + 1) * 512
> >  				    + lh.payload_offset,
> > -				    lh.payload_length - 4);
> > +				    lh.payload_length);
> >    if (!off_file)
> >      goto fail;
> >  
> > 
> Looks strange indeed. I wonder what was original intention, code looked
> this way from the very beginning. May be it is required with some other
> format?

I also wonder that possibility. But looking into other formats it looks
to me more likely a bug than intended.

Unless there's unexpected padding in the end of payload, it looks to me
that xz would also be failed in footer sanity check as the last stream
block's footer magic is discarded by that size of payload_length - 4. 

Thanks,
Michael

Re: [PATCH] fix attempt to seek outside of the file error when parsing xen ELF notes

[Vladimir 'phcoder' Serbinenko]

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

Le jeu. 3 mars 2016 19:17, Andrei Borzenkov <arvidjaar@gmail.com> a écrit :

> 03.03.2016 10:33, Michael Chang пишет:
> > The kernel bzImage payload length appears to be incorrectly minus by 4
> when
> > trying to grub_file_offset_open for the uncompressed vmlinux image. If
> gzip
> > used as compressor it will lead to the file->size read from offset of
> crc32
> > instead of the uncompressed image size.
> >
> > [1]
> > http://www.onicos.com/staff/iz/formats/gzip.html
> > ---
> >  grub-core/loader/i386/xen_file.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/loader/i386/xen_file.c
> b/grub-core/loader/i386/xen_file.c
> > index 5836218..37f9ad8 100644
> > --- a/grub-core/loader/i386/xen_file.c
> > +++ b/grub-core/loader/i386/xen_file.c
> > @@ -55,11 +55,11 @@ grub_xen_file (grub_file_t file)
> >    grub_dprintf ("xen", "found bzimage payload 0x%llx-0x%llx\n",
> >               (unsigned long long) (lh.setup_sects + 1) * 512
> >               + lh.payload_offset,
> > -             (unsigned long long) lh.payload_length - 4);
> > +             (unsigned long long) lh.payload_length);
> >
> >    off_file = grub_file_offset_open (file, (lh.setup_sects + 1) * 512
> >                                   + lh.payload_offset,
> > -                                 lh.payload_length - 4);
> > +                                 lh.payload_length);
> >    if (!off_file)
> >      goto fail;
> >
> >
> Looks strange indeed. I wonder what was original intention, code looked
> this way from the very beginning. May be it is required with some other
> format?
>
I don't remember and I haven't kept the image I originally used for this.
So I can't check if this doesn't break anything. But it looks like it fixes
stuff, so I'm going to commit it

>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2918 bytes --]