| Message ID | 20170911140753.19067-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.09.17 at 16:07, <wei.liu2@citrix.com> wrote: > e2fc5bb5cb4 ("hvmloader: dynamically determine scratch memory range > for tests") makes the test dependent on _end. Coverity reported that > the shift might overflow and suggested we cast i to uint64_t. > > Coverity-ID: 1417660 > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > tools/firmware/hvmloader/tests.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/firmware/hvmloader/tests.c > b/tools/firmware/hvmloader/tests.c > index a70c72dffb..3c4c29a6c7 100644 > --- a/tools/firmware/hvmloader/tests.c > +++ b/tools/firmware/hvmloader/tests.c > @@ -231,7 +231,7 @@ static int shadow_gs_test(void) > pd += 512; > /* Level 2: */ > for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 9); i++ ) With the shift here there's no way ... > - *pd++ = (i << (PAGE_SHIFT + 9)) + 0x1e3; > + *pd++ = ((uint64_t)i << (PAGE_SHIFT + 9)) + 0x1e3; ... this shift (or the add) can overflow, irrespective of the actual value of _end, and with my dislike of casts I'm a little hesitant to give my ack for such a tool weakness workaround. Jan
On Mon, Sep 11, 2017 at 08:55:53AM -0600, Jan Beulich wrote: > >>> On 11.09.17 at 16:07, <wei.liu2@citrix.com> wrote: > > e2fc5bb5cb4 ("hvmloader: dynamically determine scratch memory range > > for tests") makes the test dependent on _end. Coverity reported that > > the shift might overflow and suggested we cast i to uint64_t. > > > > Coverity-ID: 1417660 > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > tools/firmware/hvmloader/tests.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/firmware/hvmloader/tests.c > > b/tools/firmware/hvmloader/tests.c > > index a70c72dffb..3c4c29a6c7 100644 > > --- a/tools/firmware/hvmloader/tests.c > > +++ b/tools/firmware/hvmloader/tests.c > > @@ -231,7 +231,7 @@ static int shadow_gs_test(void) > > pd += 512; > > /* Level 2: */ > > for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 9); i++ ) > > With the shift here there's no way ... > > > - *pd++ = (i << (PAGE_SHIFT + 9)) + 0x1e3; > > + *pd++ = ((uint64_t)i << (PAGE_SHIFT + 9)) + 0x1e3; > > ... this shift (or the add) can overflow, irrespective of the actual > value of _end, and with my dislike of casts I'm a little hesitant to > give my ack for such a tool weakness workaround. We can also mark that as false positive. It's you and Andrew's call. I'm indifferent to the final solution.
>>> On 11.09.17 at 17:01, <wei.liu2@citrix.com> wrote: > On Mon, Sep 11, 2017 at 08:55:53AM -0600, Jan Beulich wrote: >> >>> On 11.09.17 at 16:07, <wei.liu2@citrix.com> wrote: >> > e2fc5bb5cb4 ("hvmloader: dynamically determine scratch memory range >> > for tests") makes the test dependent on _end. Coverity reported that >> > the shift might overflow and suggested we cast i to uint64_t. >> > >> > Coverity-ID: 1417660 >> > >> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> >> > --- >> > Cc: Jan Beulich <jbeulich@suse.com> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> > --- >> > tools/firmware/hvmloader/tests.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/tools/firmware/hvmloader/tests.c >> > b/tools/firmware/hvmloader/tests.c >> > index a70c72dffb..3c4c29a6c7 100644 >> > --- a/tools/firmware/hvmloader/tests.c >> > +++ b/tools/firmware/hvmloader/tests.c >> > @@ -231,7 +231,7 @@ static int shadow_gs_test(void) >> > pd += 512; >> > /* Level 2: */ >> > for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 9); i++ ) >> >> With the shift here there's no way ... >> >> > - *pd++ = (i << (PAGE_SHIFT + 9)) + 0x1e3; >> > + *pd++ = ((uint64_t)i << (PAGE_SHIFT + 9)) + 0x1e3; >> >> ... this shift (or the add) can overflow, irrespective of the actual >> value of _end, and with my dislike of casts I'm a little hesitant to >> give my ack for such a tool weakness workaround. > > We can also mark that as false positive. I would prefer that. > It's you and Andrew's call. Andrew? Jan
On Mon, Sep 11, 2017 at 09:20:58AM -0600, Jan Beulich wrote: > >>> On 11.09.17 at 17:01, <wei.liu2@citrix.com> wrote: > > On Mon, Sep 11, 2017 at 08:55:53AM -0600, Jan Beulich wrote: > >> >>> On 11.09.17 at 16:07, <wei.liu2@citrix.com> wrote: > >> > e2fc5bb5cb4 ("hvmloader: dynamically determine scratch memory range > >> > for tests") makes the test dependent on _end. Coverity reported that > >> > the shift might overflow and suggested we cast i to uint64_t. > >> > > >> > Coverity-ID: 1417660 > >> > > >> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > >> > --- > >> > Cc: Jan Beulich <jbeulich@suse.com> > >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> > --- > >> > tools/firmware/hvmloader/tests.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/tools/firmware/hvmloader/tests.c > >> > b/tools/firmware/hvmloader/tests.c > >> > index a70c72dffb..3c4c29a6c7 100644 > >> > --- a/tools/firmware/hvmloader/tests.c > >> > +++ b/tools/firmware/hvmloader/tests.c > >> > @@ -231,7 +231,7 @@ static int shadow_gs_test(void) > >> > pd += 512; > >> > /* Level 2: */ > >> > for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 9); i++ ) > >> > >> With the shift here there's no way ... > >> > >> > - *pd++ = (i << (PAGE_SHIFT + 9)) + 0x1e3; > >> > + *pd++ = ((uint64_t)i << (PAGE_SHIFT + 9)) + 0x1e3; > >> > >> ... this shift (or the add) can overflow, irrespective of the actual > >> value of _end, and with my dislike of casts I'm a little hesitant to > >> give my ack for such a tool weakness workaround. > > > > We can also mark that as false positive. > > I would prefer that. > > > It's you and Andrew's call. > > Andrew? > I have marked it as false positive with appropriate comment added to save everybody some time.
diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c index a70c72dffb..3c4c29a6c7 100644 --- a/tools/firmware/hvmloader/tests.c +++ b/tools/firmware/hvmloader/tests.c @@ -231,7 +231,7 @@ static int shadow_gs_test(void) pd += 512; /* Level 2: */ for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 9); i++ ) - *pd++ = (i << (PAGE_SHIFT + 9)) + 0x1e3; + *pd++ = ((uint64_t)i << (PAGE_SHIFT + 9)) + 0x1e3; asm volatile ( /* CR4.PAE=1 */
e2fc5bb5cb4 ("hvmloader: dynamically determine scratch memory range for tests") makes the test dependent on _end. Coverity reported that the shift might overflow and suggested we cast i to uint64_t. Coverity-ID: 1417660 Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- tools/firmware/hvmloader/tests.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)