Skip to contentSkip to conversation

hvmloader: cast to avoid potential overflow in shadow_gs_test

PATCH In review Wei Liu 2 participants 5 messages Sep 11, 2017 – Sep 12, 2017

lore
Share & tools
lore

Wei Liu

author
#0 lore raw

PATCH 1/1 hvmloader: cast to avoid potential overflow in shadow_gs_test

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 <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/firmware/hvmloader/tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/firmware/hvmloader/tests.c +1 -1
index a70c72dffb..3c4c29a6c7 100644
@@ -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 */
Signature · 7 lines
2.11.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#1 lore raw
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 16:07, <[email protected]> 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 <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/firmware/hvmloader/tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/firmware/hvmloader/tests.c +0 -0
index a70c72dffb..3c4c29a6c7 100644

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 233

Quoted 3 lines
tools/firmware/hvmloader/tests.c +0 -0
@@ -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 ...

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 234

Quoted 2 lines
tools/firmware/hvmloader/tests.c +1 -1
-        *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

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

author
#2 lore raw
Quoted 27 lines · Jan Beulich
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 16:07, <[email protected]> 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 <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/firmware/hvmloader/tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/firmware/hvmloader/tests.c +0 -0
index a70c72dffb..3c4c29a6c7 100644

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 233

Quoted 3 lines
tools/firmware/hvmloader/tests.c +0 -0
@@ -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 ...

Quoted 6 lines

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 234

Quoted 2 lines
tools/firmware/hvmloader/tests.c +1 -1
-        *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.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#3 lore raw
Quoted 29 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 17:01, <[email protected]> wrote:
Quoted 27 lines · Jan Beulich
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 16:07, <[email protected]> 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 <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/firmware/hvmloader/tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/firmware/hvmloader/tests.c +0 -0
index a70c72dffb..3c4c29a6c7 100644

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 233

Quoted 3 lines
tools/firmware/hvmloader/tests.c +0 -0
@@ -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 ...

Quoted 8 lines
Quoted 6 lines

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 234

Quoted 2 lines
tools/firmware/hvmloader/tests.c +1 -1
-        *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.

Quoted 1 line

It's you and Andrew's call.

Andrew?

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

author
#4 lore raw
Quoted 36 lines · Jan Beulich
Quoted 36 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 17:01, <[email protected]> wrote:
Quoted 33 lines · Jan Beulich
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 11.09.17 at 16:07, <[email protected]> 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 <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/firmware/hvmloader/tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/firmware/hvmloader/tests.c +0 -0
index a70c72dffb..3c4c29a6c7 100644

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 233

Quoted 3 lines
tools/firmware/hvmloader/tests.c +0 -0
@@ -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 ...

tools/firmware/hvmloader/tests.c static int shadow_gs_test(void) · line 234

Quoted 2 lines
tools/firmware/hvmloader/tests.c +1 -1
-        *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.

Quoted 8 lines
Quoted 1 line

We can also mark that as false positive.

I would prefer that.

Quoted 1 line

It's you and Andrew's call.

Andrew?

I have marked it as false positive with appropriate comment added to save everybody some time.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel