Change in coreboot[master]: libpayload/cbgfx: Fix overflow in transform_vector()

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

libpayload/cbgfx: Fix overflow in transform_vector()

Fix potential overflow when multiplying integers in transform_vector().

In addition, check the lower bound in within_box().

BRANCH=none
BUG=b:146399181, b:159772149
TEST=emerge-puff libpayload
TEST=Previous screen is cleared properly for menu UI

Change-Id: I57845f54e18e5bdbd0d774209ee9632cb860b0c2
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 10 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42770/1

diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c
index 81d2bb9..7c1e67f 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -113,8 +113,8 @@
 {
    if (!is_valid_scale(a))
    	return CBGFX_ERROR_INVALID_PARAMETER;
-	out->x = a->x.n * in->x / a->x.d + offset->x;
-	out->y = a->y.n * in->y / a->y.d + offset->y;
+	out->x = (int64_t)a->x.n * in->x / a->x.d + offset->x;
+	out->y = (int64_t)a->y.n * in->y / a->y.d + offset->y;
    return CBGFX_SUCCESS;
 }

@@ -124,11 +124,15 @@
  */
 static int within_box(const struct vector *v, const struct rect *bound)
 {
-	if (v->x < bound->offset.x + bound->size.width &&
-			v->y < bound->offset.y + bound->size.height)
+	if (v->x > bound->offset.x &&
+	    v->y > bound->offset.y &&
+	    v->x < bound->offset.x + bound->size.width &&
+	    v->y < bound->offset.y + bound->size.height)
    	return 1;
-	else if (v->x <= bound->offset.x + bound->size.width &&
-			v->y <= bound->offset.y + bound->size.height)
+	else if (v->x >= bound->offset.x &&
+		 v->y >= bound->offset.y &&
+		 v->x <= bound->offset.x + bound->size.width &&
+		 v->y <= bound->offset.y + bound->size.height)
    	return 0;
    else
    	return -1;

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 1: Code-Review+2

Tested this on my monitor which was not redrawing properly before.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG@16 
PS1, Line 16: TEST=Previous screen is cleared properly for menu UI
Did that fail before?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/42770/1/payloads/libpayload/drivers... 
PS1, Line 123: Note that only the right and bottom edges are examined.
Looks like you're changing this, so remove the comment?

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG@16 
PS1, Line 16: TEST=Previous screen is cleared properly for menu UI
> Did that fail before?

Yes, the screen used to not clear before redrawing the next screen and we'd see artifacts.

[Yu-Ping Wu (Code Review)]

Hello Shelley Chen, build bot (Jenkins), Joel Kitching, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42770

to look at the new patch set (#2).

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

libpayload/cbgfx: Fix overflow in transform_vector()

Fix potential overflow when multiplying integers in transform_vector().

In addition, check the lower bound in within_box().

BRANCH=none
BUG=b:146399181, b:159772149
TEST=emerge-puff libpayload
TEST=Previous screen is cleared properly for menu UI

Change-Id: I57845f54e18e5bdbd0d774209ee9632cb860b0c2
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 11 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42770/2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1/payloads/libpayload/drivers... 
File payloads/libpayload/drivers/video/graphics.c:

https://review.coreboot.org/c/coreboot/+/42770/1/payloads/libpayload/drivers... 
PS1, Line 123: Note that only the right and bottom edges are examined.
> Looks like you're changing this, so remove the comment?

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG@16 
PS1, Line 16: TEST=Previous screen is cleared properly for menu UI
> Yes, the screen used to not clear before redrawing the next screen and we'd see artifacts.

Thanks for the confirmation. Could you please state the problem explicitly in the commit message?

[Yu-Ping Wu (Code Review)]

Hello Shelley Chen, build bot (Jenkins), Joel Kitching, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42770

to look at the new patch set (#3).

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

libpayload/cbgfx: Fix overflow in transform_vector()

Fix potential overflow when multiplying integers in transform_vector().
This issue is causing the absolute coordinate of the bottom right corner
of the box to be incorrectly calculated for draw_rounded_box(), which is
used in menu UI to clear the previous screen.

In addition, check the lower bound in within_box().

BRANCH=none
BUG=b:146399181, b:159772149
TEST=emerge-puff libpayload
TEST=Previous screen is cleared properly for menu UI

Change-Id: I57845f54e18e5bdbd0d774209ee9632cb860b0c2
Signed-off-by: Yu-Ping Wu 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 11 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42770/3

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/42770/1//COMMIT_MSG@16 
PS1, Line 16: TEST=Previous screen is cleared properly for menu UI
> Thanks for the confirmation. […]

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 3: Code-Review+1

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42770 )

Change subject: libpayload/cbgfx: Fix overflow in transform_vector()
......................................................................

libpayload/cbgfx: Fix overflow in transform_vector()

Fix potential overflow when multiplying integers in transform_vector().
This issue is causing the absolute coordinate of the bottom right corner
of the box to be incorrectly calculated for draw_rounded_box(), which is
used in menu UI to clear the previous screen.

In addition, check the lower bound in within_box().

BRANCH=none
BUG=b:146399181, b:159772149
TEST=emerge-puff libpayload
TEST=Previous screen is cleared properly for menu UI

Change-Id: I57845f54e18e5bdbd0d774209ee9632cb860b0c2
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42770
Reviewed-by: Paul Menzel 
Reviewed-by: Shelley Chen 
Tested-by: build bot (Jenkins) 
---
M payloads/libpayload/drivers/video/graphics.c
1 file changed, 11 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Shelley Chen: Looks good to me, approved

diff --git a/payloads/libpayload/drivers/video/graphics.c b/payloads/libpayload/drivers/video/graphics.c
index 81d2bb9..13eac28 100644
--- a/payloads/libpayload/drivers/video/graphics.c
+++ b/payloads/libpayload/drivers/video/graphics.c
@@ -113,22 +113,26 @@
 {
    if (!is_valid_scale(a))
    	return CBGFX_ERROR_INVALID_PARAMETER;
-	out->x = a->x.n * in->x / a->x.d + offset->x;
-	out->y = a->y.n * in->y / a->y.d + offset->y;
+	out->x = (int64_t)a->x.n * in->x / a->x.d + offset->x;
+	out->y = (int64_t)a->y.n * in->y / a->y.d + offset->y;
    return CBGFX_SUCCESS;
 }

/*
  * Returns 1 if v is exclusively within box, 0 if v is inclusively within box,
- * or -1 otherwise. Note that only the right and bottom edges are examined.
+ * or -1 otherwise.
  */
 static int within_box(const struct vector *v, const struct rect *bound)
 {
-	if (v->x < bound->offset.x + bound->size.width &&
-			v->y < bound->offset.y + bound->size.height)
+	if (v->x > bound->offset.x &&
+	    v->y > bound->offset.y &&
+	    v->x < bound->offset.x + bound->size.width &&
+	    v->y < bound->offset.y + bound->size.height)
    	return 1;
-	else if (v->x <= bound->offset.x + bound->size.width &&
-			v->y <= bound->offset.y + bound->size.height)
+	else if (v->x >= bound->offset.x &&
+		 v->y >= bound->offset.y &&
+		 v->x <= bound->offset.x + bound->size.width &&
+		 v->y <= bound->offset.y + bound->size.height)
    	return 0;
    else
    	return -1;