Change in coreboot[master]: soc/intel/baytrail: Fix undefined behaviour in integer shifts
Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Fix undefined behaviour in integer shifts ......................................................................
soc/intel/baytrail: Fix undefined behaviour in integer shifts
Shifting a bit into the highest position of an 'int' is undefined behaviour (since it counts as integer overflow), and this manifests itself in two ways in the following code:
First, gpu_pipe{a,b}_port_select are defined as int, and can have the values 1 or 2. In the case when they have the value 2, the shift 2 << 30 will be undefined, as described above. Change these variables to something more reasonable like a uint8_t, which is unsigned.
Second, in any bit shift, any variable with width less than an int will be implicitly promoted to an int before performing the bit shift, which introduces the undefined behaviour problems from above. For example, the variable gpu_pipea_power_on_delay is a uint16_t, and if its highest bit is set, the shift gpu_pipea_power_on_delay << 16 will be undefined. To prevent this, cast all smaller variables to a u32 before the shift, which will prevent the implicit promotion madness.
Change-Id: Ic5db6001504cefb501dee199590a0e961a15771b Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229{699,700,701,702} --- M src/soc/intel/baytrail/chip.h M src/soc/intel/baytrail/gfx.c 2 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34487/1
diff --git a/src/soc/intel/baytrail/chip.h b/src/soc/intel/baytrail/chip.h index 7736e72..00e7fd6 100644 --- a/src/soc/intel/baytrail/chip.h +++ b/src/soc/intel/baytrail/chip.h @@ -69,7 +69,7 @@ /* Allow PCIe devices to wake system from suspend. */ int pcie_wake_enable;
- int gpu_pipea_port_select; /* Port select: 1=DP_B 2=DP_C */ + uint8_t gpu_pipea_port_select; /* Port select: 1=DP_B 2=DP_C */ uint16_t gpu_pipea_power_on_delay; uint16_t gpu_pipea_light_on_delay; uint16_t gpu_pipea_power_off_delay; @@ -77,7 +77,7 @@ uint16_t gpu_pipea_power_cycle_delay; int gpu_pipea_pwm_freq_hz;
- int gpu_pipeb_port_select; /* Port select: 1=DP_B 2=DP_C */ + uint8_t gpu_pipeb_port_select; /* Port select: 1=DP_B 2=DP_C */ uint16_t gpu_pipeb_power_on_delay; uint16_t gpu_pipeb_light_on_delay; uint16_t gpu_pipeb_power_off_delay; diff --git a/src/soc/intel/baytrail/gfx.c b/src/soc/intel/baytrail/gfx.c index 4a79991..5100c8e 100644 --- a/src/soc/intel/baytrail/gfx.c +++ b/src/soc/intel/baytrail/gfx.c @@ -320,13 +320,13 @@ PP_CONTROL_UNLOCK | PP_CONTROL_EDP_FORCE_VDD), /* POWER ON */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_ON_DELAYS), - (config->gpu_pipea_port_select << 30 | - config->gpu_pipea_power_on_delay << 16 | - config->gpu_pipea_light_on_delay)), + ((u32)config->gpu_pipea_port_select << 30 | + (u32)config->gpu_pipea_power_on_delay << 16 | + (u32)config->gpu_pipea_light_on_delay)), /* POWER OFF */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_OFF_DELAYS), - (config->gpu_pipea_power_off_delay << 16 | - config->gpu_pipea_light_off_delay)), + ((u32)config->gpu_pipea_power_off_delay << 16 | + (u32)config->gpu_pipea_light_off_delay)), /* DIVISOR */ REG_RES_RMW32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_DIVISOR), ~0x1f, config->gpu_pipea_power_cycle_delay), @@ -338,13 +338,13 @@ PP_CONTROL_UNLOCK | PP_CONTROL_EDP_FORCE_VDD), /* POWER ON */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_ON_DELAYS), - (config->gpu_pipeb_port_select << 30 | - config->gpu_pipeb_power_on_delay << 16 | - config->gpu_pipeb_light_on_delay)), + ((u32)config->gpu_pipeb_port_select << 30 | + (u32)config->gpu_pipeb_power_on_delay << 16 | + (u32)config->gpu_pipeb_light_on_delay)), /* POWER OFF */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_OFF_DELAYS), - (config->gpu_pipeb_power_off_delay << 16 | - config->gpu_pipeb_light_off_delay)), + ((u32)config->gpu_pipeb_power_off_delay << 16 | + (u32)config->gpu_pipeb_light_off_delay)), /* DIVISOR */ REG_RES_RMW32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_DIVISOR), ~0x1f, config->gpu_pipeb_power_cycle_delay),
Hello Kyösti Mälkki, Patrick Rudolph, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34487
to look at the new patch set (#2).
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
soc/intel/baytrail: Prevent unintended sign extensions
Consider the following assignment:
u64 = s32
For positive values this is fine, but if the s32 is negative, it will be sign-extended in the conversion to a very large unsigned integer. This manifests itself in two ways in the following code:
First, gpu_pipe{a,b}_port_select are defined as int, and can have the values 1 or 2. In the case when they have the value 2, the shift 2 << 30 will be a negative number, making it susceptible to the sign-extension problem above. Change these variables to something more reasonable like a uint8_t, which is unsigned.
Second, in any bit shift, any variable with width less than an int will be implicitly promoted to an int before performing the bit shift. For example, the variable gpu_pipea_power_on_delay is a uint16_t, and if its highest bit is set, the shift gpu_pipea_power_on_delay << 16 will become negative, again introducing the above problem. To prevent this, cast all smaller variables to a u32 before the shift, which will prevent the implicit promotions and sign extensions.
Change-Id: Ic5db6001504cefb501dee199590a0e961a15771b Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229{699,700,701,702} --- M src/soc/intel/baytrail/chip.h M src/soc/intel/baytrail/gfx.c 2 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34487/2
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34487/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34487/2//COMMIT_MSG@33 PS2, Line 33: Found-by: Coverity CID 1229{699,700,701,702} I really like this syntax. But using the full syntax makes it easier to search with grep for it. Can you extend it?
Hello Alexander Couzens, Kyösti Mälkki, Patrick Rudolph, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34487
to look at the new patch set (#3).
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
soc/intel/baytrail: Prevent unintended sign extensions
Consider the following assignment:
u64 = s32
For positive values this is fine, but if the s32 is negative, it will be sign-extended in the conversion to a very large unsigned integer. This manifests itself in two ways in the following code:
First, gpu_pipe{a,b}_port_select are defined as int, and can have the values 1 or 2. In the case when they have the value 2, the shift 2 << 30 will be a negative number, making it susceptible to the sign-extension problem above. Change these variables to something more reasonable like a uint8_t, which is unsigned.
Second, in any bit shift, any variable with width less than an int will be implicitly promoted to an int before performing the bit shift. For example, the variable gpu_pipea_power_on_delay is a uint16_t, and if its highest bit is set, the shift gpu_pipea_power_on_delay << 16 will become negative, again introducing the above problem. To prevent this, cast all smaller variables to a u32 before the shift, which will prevent the implicit promotions and sign extensions.
Change-Id: Ic5db6001504cefb501dee199590a0e961a15771b Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229699, 1229700, 1229701, 1229702 --- M src/soc/intel/baytrail/chip.h M src/soc/intel/baytrail/gfx.c 2 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34487/3
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34487/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34487/2//COMMIT_MSG@33 PS2, Line 33: Found-by: Coverity CID 1229{699,700,701,702}
I really like this syntax. But using the full syntax makes it easier to search with grep for it. […]
Yup, of course
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
Patch Set 3: Code-Review+2
I would test with my now-booting baytrail potato (asrock q1900m) but I haven't got graphics working yet sooo... It should work though :)
Alexander Couzens has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34487 )
Change subject: soc/intel/baytrail: Prevent unintended sign extensions ......................................................................
soc/intel/baytrail: Prevent unintended sign extensions
Consider the following assignment:
u64 = s32
For positive values this is fine, but if the s32 is negative, it will be sign-extended in the conversion to a very large unsigned integer. This manifests itself in two ways in the following code:
First, gpu_pipe{a,b}_port_select are defined as int, and can have the values 1 or 2. In the case when they have the value 2, the shift 2 << 30 will be a negative number, making it susceptible to the sign-extension problem above. Change these variables to something more reasonable like a uint8_t, which is unsigned.
Second, in any bit shift, any variable with width less than an int will be implicitly promoted to an int before performing the bit shift. For example, the variable gpu_pipea_power_on_delay is a uint16_t, and if its highest bit is set, the shift gpu_pipea_power_on_delay << 16 will become negative, again introducing the above problem. To prevent this, cast all smaller variables to a u32 before the shift, which will prevent the implicit promotions and sign extensions.
Change-Id: Ic5db6001504cefb501dee199590a0e961a15771b Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229699, 1229700, 1229701, 1229702 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34487 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Alexander Couzens lynxis@fe80.eu Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/baytrail/chip.h M src/soc/intel/baytrail/gfx.c 2 files changed, 12 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Alexander Couzens: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/baytrail/chip.h b/src/soc/intel/baytrail/chip.h index 7736e72..00e7fd6 100644 --- a/src/soc/intel/baytrail/chip.h +++ b/src/soc/intel/baytrail/chip.h @@ -69,7 +69,7 @@ /* Allow PCIe devices to wake system from suspend. */ int pcie_wake_enable;
- int gpu_pipea_port_select; /* Port select: 1=DP_B 2=DP_C */ + uint8_t gpu_pipea_port_select; /* Port select: 1=DP_B 2=DP_C */ uint16_t gpu_pipea_power_on_delay; uint16_t gpu_pipea_light_on_delay; uint16_t gpu_pipea_power_off_delay; @@ -77,7 +77,7 @@ uint16_t gpu_pipea_power_cycle_delay; int gpu_pipea_pwm_freq_hz;
- int gpu_pipeb_port_select; /* Port select: 1=DP_B 2=DP_C */ + uint8_t gpu_pipeb_port_select; /* Port select: 1=DP_B 2=DP_C */ uint16_t gpu_pipeb_power_on_delay; uint16_t gpu_pipeb_light_on_delay; uint16_t gpu_pipeb_power_off_delay; diff --git a/src/soc/intel/baytrail/gfx.c b/src/soc/intel/baytrail/gfx.c index 4a79991..5100c8e 100644 --- a/src/soc/intel/baytrail/gfx.c +++ b/src/soc/intel/baytrail/gfx.c @@ -320,13 +320,13 @@ PP_CONTROL_UNLOCK | PP_CONTROL_EDP_FORCE_VDD), /* POWER ON */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_ON_DELAYS), - (config->gpu_pipea_port_select << 30 | - config->gpu_pipea_power_on_delay << 16 | - config->gpu_pipea_light_on_delay)), + ((u32)config->gpu_pipea_port_select << 30 | + (u32)config->gpu_pipea_power_on_delay << 16 | + (u32)config->gpu_pipea_light_on_delay)), /* POWER OFF */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_OFF_DELAYS), - (config->gpu_pipea_power_off_delay << 16 | - config->gpu_pipea_light_off_delay)), + ((u32)config->gpu_pipea_power_off_delay << 16 | + (u32)config->gpu_pipea_light_off_delay)), /* DIVISOR */ REG_RES_RMW32(PCI_BASE_ADDRESS_0, PIPEA_REG(PP_DIVISOR), ~0x1f, config->gpu_pipea_power_cycle_delay), @@ -338,13 +338,13 @@ PP_CONTROL_UNLOCK | PP_CONTROL_EDP_FORCE_VDD), /* POWER ON */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_ON_DELAYS), - (config->gpu_pipeb_port_select << 30 | - config->gpu_pipeb_power_on_delay << 16 | - config->gpu_pipeb_light_on_delay)), + ((u32)config->gpu_pipeb_port_select << 30 | + (u32)config->gpu_pipeb_power_on_delay << 16 | + (u32)config->gpu_pipeb_light_on_delay)), /* POWER OFF */ REG_RES_WRITE32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_OFF_DELAYS), - (config->gpu_pipeb_power_off_delay << 16 | - config->gpu_pipeb_light_off_delay)), + ((u32)config->gpu_pipeb_power_off_delay << 16 | + (u32)config->gpu_pipeb_light_off_delay)), /* DIVISOR */ REG_RES_RMW32(PCI_BASE_ADDRESS_0, PIPEB_REG(PP_DIVISOR), ~0x1f, config->gpu_pipeb_power_cycle_delay),
-
Alexander Couzens (Code Review) -
Angel Pons (Code Review)
-
Jacob Garber (Code Review)
-
Patrick Georgi (Code Review)