Libguestfs - Feb 2017 - Re: [PATCH v2 1/2] lib: change how hbin sections are read.

On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski
wrote:
> Correct, however there's also no guarantee that seeking by 4k in
> "garbage" data would not land you in registry data that happens
to
> evaluate to "hbin" as well. That's why I put "hbin"
offset validation
> check couple of lines below to make sure that the "hbin" we found
by
> searching is a proper one. The offset check I'm referring to is:
> 
> /* get "stated" hbin offset from header */
> size_t page_offset = le32to(page->offset_first) + 0x1000;
> 
> /* if that does not match our current file offset,
>    then exit with error */
> if (page_offset != off) { 
>   SET_ERRNO...
> }
Still, what kind of corruption would move a genuine hbin to a
non-page-sized offset in the file?  It seems unlikely to me ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones
wrote:
> On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski wrote:
> > Correct, however there's also no guarantee that seeking by 4k in
> > "garbage" data would not land you in registry data that
happens to
> > evaluate to "hbin" as well. That's why I put
"hbin" offset
> > validation
> > check couple of lines below to make sure that the "hbin" we
found
> > by
> > searching is a proper one. The offset check I'm referring to is:
> > 
> > /* get "stated" hbin offset from header */
> > size_t page_offset = le32to(page->offset_first) + 0x1000;
> > 
> > /* if that does not match our current file offset,
> >    then exit with error */
> > if (page_offset != off) { 
> >   SET_ERRNO...
> > }
> 
> Still, what kind of corruption would move a genuine hbin to a
> non-page-sized offset in the file?  It seems unlikely to me ...
> 
> Rich.
> 
None that I know of :-) However I don't see any other simple way to
silence that GCC7 warning and it seems harmless that way to me (besides
the fact that the loop will have to make more iterations)

On Wed, 2017-02-15 at 16:50 -0500, Dawid Zamirski wrote:
> On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
> > On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski wrote:
> > > Correct, however there's also no guarantee that seeking by 4k
in
> > > "garbage" data would not land you in registry data that
happens
> > > to
> > > evaluate to "hbin" as well. That's why I put
"hbin" offset
> > > validation
> > > check couple of lines below to make sure that the
"hbin" we found
> > > by
> > > searching is a proper one. The offset check I'm referring to
is:
> > > 
> > > /* get "stated" hbin offset from header */
> > > size_t page_offset = le32to(page->offset_first) + 0x1000;
> > > 
> > > /* if that does not match our current file offset,
> > >    then exit with error */
> > > if (page_offset != off) { 
> > >   SET_ERRNO...
> > > }
> > 
> > Still, what kind of corruption would move a genuine hbin to a
> > non-page-sized offset in the file?  It seems unlikely to me ...
> > 
> > Rich.
> > 
> 
> None that I know of :-) However I don't see any other simple way to
> silence that GCC7 warning and it seems harmless that way to me
> (besides
> the fact that the loop will have to make more iterations)
So I've found a way to keep offsetting by 4k and keep GCC7 happy:

while (off < h->endpages) {
  if (off + 0x1000 > off)
    off += 0x1000;
  else
    break; // off would overflow

Is this acceptable?

Regards,
Dawid