* [PATCH 0/2] hvmloader: fix two issues spotted by Coverity @ 2016-08-19 8:06 Wei Liu 2016-08-19 8:06 ` [PATCH 1/2] hvmloader: correctly copy signature to info structures Wei Liu 2016-08-19 8:06 ` [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry Wei Liu 0 siblings, 2 replies; 11+ messages in thread From: Wei Liu @ 2016-08-19 8:06 UTC (permalink / raw) To: Xen-devel; +Cc: Anthony PERARD, Andrew Cooper, Wei Liu, Jan Beulich Wei Liu (2): hvmloader: correctly copy signature to info structures hvmloader: cast to 64bit before multiplication in get_module_entry tools/firmware/hvmloader/hvmloader.c | 4 ++-- tools/firmware/hvmloader/ovmf.c | 3 ++- tools/firmware/hvmloader/seabios.c | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] hvmloader: correctly copy signature to info structures 2016-08-19 8:06 [PATCH 0/2] hvmloader: fix two issues spotted by Coverity Wei Liu @ 2016-08-19 8:06 ` Wei Liu 2016-08-19 8:25 ` Jan Beulich 2016-08-19 8:06 ` [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry Wei Liu 1 sibling, 1 reply; 11+ messages in thread From: Wei Liu @ 2016-08-19 8:06 UTC (permalink / raw) To: Xen-devel; +Cc: Anthony PERARD, Andrew Cooper, Wei Liu, Jan Beulich The original code used sizeof(info->signature) as the size parameter for memcpy, which was wrong. Fix that by calculating the correct size. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- tools/firmware/hvmloader/ovmf.c | 3 ++- tools/firmware/hvmloader/seabios.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c index b4bcc93..a4ed661 100644 --- a/tools/firmware/hvmloader/ovmf.c +++ b/tools/firmware/hvmloader/ovmf.c @@ -67,10 +67,11 @@ struct ovmf_info { static void ovmf_setup_bios_info(void) { struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS; + const char sig[] = "XenHVMOVMF"; memset(info, 0, sizeof(*info)); - memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature)); + memcpy(info->signature, sig, sizeof(sig)); info->length = sizeof(*info); } diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c index 5c9a351..ca092cc 100644 --- a/tools/firmware/hvmloader/seabios.c +++ b/tools/firmware/hvmloader/seabios.c @@ -55,10 +55,11 @@ struct seabios_info { static void seabios_setup_bios_info(void) { struct seabios_info *info = (void *)BIOS_INFO_PHYSICAL_ADDRESS; + const char sig[] = "XenHVMSeaBIOS"; memset(info, 0, sizeof(*info)); - memcpy(info->signature, "XenHVMSeaBIOS", sizeof(info->signature)); + memcpy(info->signature, sig, sizeof(sig)); info->length = sizeof(*info); info->tables = (uint32_t)scratch_alloc(MAX_TABLES*sizeof(uint32_t), 0); -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures 2016-08-19 8:06 ` [PATCH 1/2] hvmloader: correctly copy signature to info structures Wei Liu @ 2016-08-19 8:25 ` Jan Beulich 2016-08-19 9:42 ` Andrew Cooper 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2016-08-19 8:25 UTC (permalink / raw) To: Wei Liu; +Cc: Anthony PERARD, Andrew Cooper, Xen-devel >>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote: > --- a/tools/firmware/hvmloader/ovmf.c > +++ b/tools/firmware/hvmloader/ovmf.c > @@ -67,10 +67,11 @@ struct ovmf_info { > static void ovmf_setup_bios_info(void) > { > struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS; > + const char sig[] = "XenHVMOVMF"; > > memset(info, 0, sizeof(*info)); > > - memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature)); > + memcpy(info->signature, sig, sizeof(sig)); > info->length = sizeof(*info); > } I think using strncpy() would be more natural in cases like this, as it would at once make clear that the destination can't be overrun no matter how large the string literal. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures 2016-08-19 8:25 ` Jan Beulich @ 2016-08-19 9:42 ` Andrew Cooper 2016-08-19 11:58 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Andrew Cooper @ 2016-08-19 9:42 UTC (permalink / raw) To: Jan Beulich, Wei Liu; +Cc: Anthony PERARD, Xen-devel On 19/08/16 09:25, Jan Beulich wrote: >>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote: >> --- a/tools/firmware/hvmloader/ovmf.c >> +++ b/tools/firmware/hvmloader/ovmf.c >> @@ -67,10 +67,11 @@ struct ovmf_info { >> static void ovmf_setup_bios_info(void) >> { >> struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS; >> + const char sig[] = "XenHVMOVMF"; >> >> memset(info, 0, sizeof(*info)); >> >> - memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature)); >> + memcpy(info->signature, sig, sizeof(sig)); >> info->length = sizeof(*info); >> } > I think using strncpy() would be more natural in cases like this, > as it would at once make clear that the destination can't be > overrun no matter how large the string literal. How about structure assignment? *info = (struct ovmf_info) { .signature = "XenHVMOVMF", .length = sizeof(*info) } which also subsumed the memset()? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures 2016-08-19 9:42 ` Andrew Cooper @ 2016-08-19 11:58 ` Jan Beulich 0 siblings, 0 replies; 11+ messages in thread From: Jan Beulich @ 2016-08-19 11:58 UTC (permalink / raw) To: Andrew Cooper, Wei Liu; +Cc: Anthony PERARD, Xen-devel >>> On 19.08.16 at 11:42, <andrew.cooper3@citrix.com> wrote: > On 19/08/16 09:25, Jan Beulich wrote: >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote: >>> --- a/tools/firmware/hvmloader/ovmf.c >>> +++ b/tools/firmware/hvmloader/ovmf.c >>> @@ -67,10 +67,11 @@ struct ovmf_info { >>> static void ovmf_setup_bios_info(void) >>> { >>> struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS; >>> + const char sig[] = "XenHVMOVMF"; >>> >>> memset(info, 0, sizeof(*info)); >>> >>> - memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature)); >>> + memcpy(info->signature, sig, sizeof(sig)); >>> info->length = sizeof(*info); >>> } >> I think using strncpy() would be more natural in cases like this, >> as it would at once make clear that the destination can't be >> overrun no matter how large the string literal. > > How about structure assignment? > > *info = (struct ovmf_info) { .signature = "XenHVMOVMF", .length = > sizeof(*info) } > > which also subsumed the memset()? Fine with me, albeit maybe a little uglier to read. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-19 8:06 [PATCH 0/2] hvmloader: fix two issues spotted by Coverity Wei Liu 2016-08-19 8:06 ` [PATCH 1/2] hvmloader: correctly copy signature to info structures Wei Liu @ 2016-08-19 8:06 ` Wei Liu 2016-08-19 8:31 ` Jan Beulich 1 sibling, 1 reply; 11+ messages in thread From: Wei Liu @ 2016-08-19 8:06 UTC (permalink / raw) To: Xen-devel; +Cc: Anthony PERARD, Andrew Cooper, Wei Liu, Jan Beulich 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 <wei.liu2@citrix.com> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-19 8:06 ` [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry Wei Liu @ 2016-08-19 8:31 ` Jan Beulich 2016-08-19 10:09 ` Andrew Cooper 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2016-08-19 8:31 UTC (permalink / raw) To: Wei Liu; +Cc: Anthony PERARD, Andrew Cooper, Xen-devel >>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-19 8:31 ` Jan Beulich @ 2016-08-19 10:09 ` Andrew Cooper 2016-08-19 12:00 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Andrew Cooper @ 2016-08-19 10:09 UTC (permalink / raw) To: Jan Beulich, Wei Liu; +Cc: Anthony PERARD, Xen-devel On 19/08/16 09:31, Jan Beulich wrote: >>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-19 10:09 ` Andrew Cooper @ 2016-08-19 12:00 ` Jan Beulich 2016-08-22 11:37 ` Wei Liu 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2016-08-19 12:00 UTC (permalink / raw) To: Andrew Cooper, Wei Liu; +Cc: Anthony PERARD, Xen-devel >>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote: > On 19/08/16 09:31, Jan Beulich wrote: >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-19 12:00 ` Jan Beulich @ 2016-08-22 11:37 ` Wei Liu 2016-08-22 11:55 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Wei Liu @ 2016-08-22 11:37 UTC (permalink / raw) To: Jan Beulich; +Cc: Anthony PERARD, Andrew Cooper, Wei Liu, Xen-devel On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote: > >>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote: > > On 19/08/16 09:31, Jan Beulich wrote: > >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry 2016-08-22 11:37 ` Wei Liu @ 2016-08-22 11:55 ` Jan Beulich 0 siblings, 0 replies; 11+ messages in thread From: Jan Beulich @ 2016-08-22 11:55 UTC (permalink / raw) To: Wei Liu; +Cc: Anthony PERARD, Andrew Cooper, Xen-devel >>> On 22.08.16 at 13:37, <wei.liu2@citrix.com> wrote: > On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote: >> >>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote: >> > On 19/08/16 09:31, Jan Beulich wrote: >> >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-08-22 12:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-19 8:06 [PATCH 0/2] hvmloader: fix two issues spotted by Coverity Wei Liu 2016-08-19 8:06 ` [PATCH 1/2] hvmloader: correctly copy signature to info structures Wei Liu 2016-08-19 8:25 ` Jan Beulich 2016-08-19 9:42 ` Andrew Cooper 2016-08-19 11:58 ` Jan Beulich 2016-08-19 8:06 ` [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry Wei Liu 2016-08-19 8:31 ` Jan Beulich 2016-08-19 10:09 ` Andrew Cooper 2016-08-19 12:00 ` Jan Beulich 2016-08-22 11:37 ` Wei Liu 2016-08-22 11:55 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.