| Message ID | 20241106174241.556373-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | eif: cope with huge section offsets | expand |
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>
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> >
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> >> >
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
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
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) {
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(+)