[v1] eif: cope with huge section offsets

[PATCH] eif: cope with huge section offsets

Posted by Paolo Bonzini 2 weeks, 3 days ago

Check for overflow to avoid that fseek() receives a sign-extended value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/osdep.h | 4 ++++
 hw/core/eif.c        | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index fe7c3c5f673..fdff07fd992 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -297,6 +297,10 @@ void QEMU_ERROR("code path is reachable")
 #error building with G_DISABLE_ASSERT is not supported
 #endif
 
+#ifndef OFF_MAX
+#define OFF_MAX (sizeof (off_t) == 8 ? INT64_MAX : INT32_MAX)
+#endif
+
 #ifndef O_LARGEFILE
 #define O_LARGEFILE 0
 #endif
diff --git a/hw/core/eif.c b/hw/core/eif.c
index 7f3b2edc9a7..cbcd80de58b 100644
--- a/hw/core/eif.c
+++ b/hw/core/eif.c
@@ -115,6 +115,10 @@ static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
 
     for (int i = 0; i < MAX_SECTIONS; ++i) {
         header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
+        if (header->section_offsets[i] > OFF_MAX) {
+            error_setg(errp, "Invalid EIF image. Section offset out of bounds");
+            return false;
+        }
     }
 
     for (int i = 0; i < MAX_SECTIONS; ++i) {
-- 
2.47.0

Re: [PATCH] eif: cope with huge section offsets

Posted by Pierrick Bouvier 2 weeks, 3 days ago


On 11/6/24 09:42, Paolo Bonzini wrote:
> Check for overflow to avoid that fseek() receives a sign-extended value.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qemu/osdep.h | 4 ++++
>   hw/core/eif.c        | 4 ++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index fe7c3c5f673..fdff07fd992 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -297,6 +297,10 @@ void QEMU_ERROR("code path is reachable")
>   #error building with G_DISABLE_ASSERT is not supported
>   #endif
>   
> +#ifndef OFF_MAX
> +#define OFF_MAX (sizeof (off_t) == 8 ? INT64_MAX : INT32_MAX)
> +#endif
> +
>   #ifndef O_LARGEFILE
>   #define O_LARGEFILE 0
>   #endif
> diff --git a/hw/core/eif.c b/hw/core/eif.c
> index 7f3b2edc9a7..cbcd80de58b 100644
> --- a/hw/core/eif.c
> +++ b/hw/core/eif.c
> @@ -115,6 +115,10 @@ static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
>   
>       for (int i = 0; i < MAX_SECTIONS; ++i) {
>           header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> +        if (header->section_offsets[i] > OFF_MAX) {

Maybe we could add a comment that sections_offsets is unsigned, as it 
can be confusing to read value > INT_MAX without more context.

> +            error_setg(errp, "Invalid EIF image. Section offset out of bounds");
> +            return false;
> +        }
>       }
>   
>       for (int i = 0; i < MAX_SECTIONS; ++i) {

Else,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH] eif: cope with huge section offsets

Posted by Paolo Bonzini 2 weeks, 3 days ago

On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:

> >       for (int i = 0; i < MAX_SECTIONS; ++i) {
> >           header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> > +        if (header->section_offsets[i] > OFF_MAX) {
>
> Maybe we could add a comment that sections_offsets is unsigned, as it
> can be confusing to read value > INT_MAX without more context.

It does sound like OFF_MAX is related to section_offsets[], but it's
actually related to off_t.  So the comparison is with the maximum
value of off_t, which is signed.

The problem would happen even if section_offsets[] was signed (for
example off_t could be 32-bit).

Paolo

> > +            error_setg(errp, "Invalid EIF image. Section offset out of bounds");
> > +            return false;
> > +        }
> >       }
> >
> >       for (int i = 0; i < MAX_SECTIONS; ++i) {
>
> Else,
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>

Re: [PATCH] eif: cope with huge section offsets

Posted by Pierrick Bouvier 2 weeks, 3 days ago

On 11/6/24 09:49, Paolo Bonzini wrote:
> On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> 
>>>        for (int i = 0; i < MAX_SECTIONS; ++i) {
>>>            header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
>>> +        if (header->section_offsets[i] > OFF_MAX) {
>>
>> Maybe we could add a comment that sections_offsets is unsigned, as it
>> can be confusing to read value > INT_MAX without more context.
> 
> It does sound like OFF_MAX is related to section_offsets[], but it's
> actually related to off_t.  So the comparison is with the maximum
> value of off_t, which is signed.
> 
> The problem would happen even if section_offsets[] was signed (for
> example off_t could be 32-bit).
> 

I'm a bit confused.
It works because section_offsets[i] is unsigned. If it was signed, and 
sizeof(off_t) is 8, we can never satisfy "(int64) > INT64_MAX".

> Paolo
> 
>>> +            error_setg(errp, "Invalid EIF image. Section offset out of bounds");
>>> +            return false;
>>> +        }
>>>        }
>>>
>>>        for (int i = 0; i < MAX_SECTIONS; ++i) {
>>
>> Else,
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>
>

Re: [PATCH] eif: cope with huge section offsets

Posted by Paolo Bonzini 2 weeks, 3 days ago

On Wed, Nov 6, 2024 at 6:54 PM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 11/6/24 09:49, Paolo Bonzini wrote:
> > On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
> > <pierrick.bouvier@linaro.org> wrote:
> >
> >>>        for (int i = 0; i < MAX_SECTIONS; ++i) {
> >>>            header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> >>> +        if (header->section_offsets[i] > OFF_MAX) {
> >>
> >> Maybe we could add a comment that sections_offsets is unsigned, as it
> >> can be confusing to read value > INT_MAX without more context.
> >
> > It does sound like OFF_MAX is related to section_offsets[], but it's
> > actually related to off_t.  So the comparison is with the maximum
> > value of off_t, which is signed.
> >
> > The problem would happen even if section_offsets[] was signed (for
> > example off_t could be 32-bit).
>
> I'm a bit confused.
> It works because section_offsets[i] is unsigned. If it was signed, and
> sizeof(off_t) is 8, we can never satisfy "(int64) > INT64_MAX".

The fact that you cannot satisfy "int64 > INT64_MAX" just means that
on this system that erroneous condition is unreachable, but it could
be reachable on others. (Actually the fact that section_offsets[] is
unsigned does matter, because otherwise you'd nede a check against 0
as well. But it doesn't matter for the correctness of *this* check
against OFF_MAX).

Paolo

Re: [PATCH] eif: cope with huge section offsets

Posted by Dorjoy Chowdhury 2 weeks, 3 days ago

On Wed, Nov 6, 2024 at 11:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Wed, Nov 6, 2024 at 6:54 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> >
> > On 11/6/24 09:49, Paolo Bonzini wrote:
> > > On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
> > > <pierrick.bouvier@linaro.org> wrote:
> > >
> > >>>        for (int i = 0; i < MAX_SECTIONS; ++i) {
> > >>>            header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> > >>> +        if (header->section_offsets[i] > OFF_MAX) {
> > >>
> > >> Maybe we could add a comment that sections_offsets is unsigned, as it
> > >> can be confusing to read value > INT_MAX without more context.
> > >
> > > It does sound like OFF_MAX is related to section_offsets[], but it's
> > > actually related to off_t.  So the comparison is with the maximum
> > > value of off_t, which is signed.
> > >
> > > The problem would happen even if section_offsets[] was signed (for
> > > example off_t could be 32-bit).
> >
> > I'm a bit confused.
> > It works because section_offsets[i] is unsigned. If it was signed, and
> > sizeof(off_t) is 8, we can never satisfy "(int64) > INT64_MAX".
>
> The fact that you cannot satisfy "int64 > INT64_MAX" just means that
> on this system that erroneous condition is unreachable, but it could
> be reachable on others. (Actually the fact that section_offsets[] is
> unsigned does matter, because otherwise you'd nede a check against 0
> as well. But it doesn't matter for the correctness of *this* check
> against OFF_MAX).
>

I think instead of putting the check for > OFF_MAX inside
read_eif_header, it would make more sense to put the check in the
read_eif_file function before the fseek line where we are actually
doing the seeking, no? What do you think?

Regards,
Dorjoy