Re: [Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
>>> On 22.08.16 at 13:37, wrote: > On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote: >> >>> On 19.08.16 at 12:09, wrote: >> > On 19/08/16 09:31, Jan Beulich wrote: >> > On 19.08.16 at 10:06, wrote: >> >>> --- a/tools/firmware/hvmloader/hvmloader.c >> >>> +++ b/tools/firmware/hvmloader/hvmloader.c >> >>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( >> >>> >> >>> if ( !modlist || >> >>> info->modlist_paddr > UINTPTR_MAX || >> >>> - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) >> >>> -> UINTPTR_MAX ) >> >>> + (info->modlist_paddr + >> >>> + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > >> >>> UINTPTR_MAX ) >> >>> return NULL; >> >> This can be had without resorting to 64-bit multiplication, by bounds >> >> checking >> >> >> >> (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist) >> >> >> >> instead. While we would certainly hope that compilers don't resort >> >> to a libgcc helper for 64-bit multiplication, I think we'd better avoid >> >> that risk altogether. >> > >> > In this case, using libgcc would cause a link error because of >> > -fno-builtin, so I don't think it is too bad. >> >> And it's this link error which I want to avoid. > > What approach should I use? I would like to clear this minor issue as > quick as possible. Well, if Andrew wants to ack the patch with the above unchanged, I won't object. I continue to prefer the suggested alternative though. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote: > >>> On 19.08.16 at 12:09, wrote: > > On 19/08/16 09:31, Jan Beulich wrote: > > On 19.08.16 at 10:06, wrote: > >>> --- a/tools/firmware/hvmloader/hvmloader.c > >>> +++ b/tools/firmware/hvmloader/hvmloader.c > >>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( > >>> > >>> if ( !modlist || > >>> info->modlist_paddr > UINTPTR_MAX || > >>> - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) > >>> -> UINTPTR_MAX ) > >>> + (info->modlist_paddr + > >>> + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > > >>> UINTPTR_MAX ) > >>> return NULL; > >> This can be had without resorting to 64-bit multiplication, by bounds > >> checking > >> > >> (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist) > >> > >> instead. While we would certainly hope that compilers don't resort > >> to a libgcc helper for 64-bit multiplication, I think we'd better avoid > >> that risk altogether. > > > > In this case, using libgcc would cause a link error because of > > -fno-builtin, so I don't think it is too bad. > > And it's this link error which I want to avoid. > What approach should I use? I would like to clear this minor issue as quick as possible. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
>>> On 19.08.16 at 12:09, wrote: > On 19/08/16 09:31, Jan Beulich wrote: > On 19.08.16 at 10:06, wrote: >>> --- a/tools/firmware/hvmloader/hvmloader.c >>> +++ b/tools/firmware/hvmloader/hvmloader.c >>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( >>> >>> if ( !modlist || >>> info->modlist_paddr > UINTPTR_MAX || >>> - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) >>> -> UINTPTR_MAX ) >>> + (info->modlist_paddr + >>> + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX >>> ) >>> return NULL; >> This can be had without resorting to 64-bit multiplication, by bounds >> checking >> >> (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist) >> >> instead. While we would certainly hope that compilers don't resort >> to a libgcc helper for 64-bit multiplication, I think we'd better avoid >> that risk altogether. > > In this case, using libgcc would cause a link error because of > -fno-builtin, so I don't think it is too bad. And it's this link error which I want to avoid. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
On 19/08/16 09:31, Jan Beulich wrote: On 19.08.16 at 10:06, wrote: >> Coverity complains: >> >> overflow_before_widen: Potentially overflowing expression >> info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is >> evaluated using 32-bit arithmetic, and then used in a context that >> expects an expression of type uint64_t (64 bits, unsigned). > To me this is Coverity splitting hair, to be honest. A very large number of security holes are because of this precise programming mistake. Coverity is doing its job correctly; it is up to a human to decide whether we care or not. > >> --- a/tools/firmware/hvmloader/hvmloader.c >> +++ b/tools/firmware/hvmloader/hvmloader.c >> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( >> >> if ( !modlist || >> info->modlist_paddr > UINTPTR_MAX || >> - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) >> -> UINTPTR_MAX ) >> + (info->modlist_paddr + >> + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX ) >> return NULL; > This can be had without resorting to 64-bit multiplication, by bounds > checking > > (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist) > > instead. While we would certainly hope that compilers don't resort > to a libgcc helper for 64-bit multiplication, I think we'd better avoid > that risk altogether. In this case, using libgcc would cause a link error because of -fno-builtin, so I don't think it is too bad. I would be surprised if we didn't have other 64bit multiplication in hvmloader. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
>>> On 19.08.16 at 10:06, wrote: > Coverity complains: > > overflow_before_widen: Potentially overflowing expression > info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is > evaluated using 32-bit arithmetic, and then used in a context that > expects an expression of type uint64_t (64 bits, unsigned). To me this is Coverity splitting hair, to be honest. > --- a/tools/firmware/hvmloader/hvmloader.c > +++ b/tools/firmware/hvmloader/hvmloader.c > @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( > > if ( !modlist || > info->modlist_paddr > UINTPTR_MAX || > - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) > -> UINTPTR_MAX ) > + (info->modlist_paddr + > + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX ) > return NULL; This can be had without resorting to 64-bit multiplication, by bounds checking (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist) instead. While we would certainly hope that compilers don't resort to a libgcc helper for 64-bit multiplication, I think we'd better avoid that risk altogether. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry
Coverity complains: overflow_before_widen: Potentially overflowing expression info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type uint64_t (64 bits, unsigned). The overflow is unlikely to happen in reality because we only expect a few modules. Fix that by casting the variable to 64bit before multiplication to placate Coverity. Signed-off-by: Wei Liu --- tools/firmware/hvmloader/hvmloader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index 7b32d86..970222c 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry( if ( !modlist || info->modlist_paddr > UINTPTR_MAX || - (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) -> UINTPTR_MAX ) + (info->modlist_paddr + + (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX ) return NULL; for ( i = 0; i < info->nr_modules; i++ ) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
