Change in ...coreboot[master]: sb/amd/sr5650: Add fine-grained bounds checking

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33458

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

sb/amd/sr5650: Add fine-grained bounds checking

The code currently checks that 4 <= dev_index <= 10, which after
subtraction by 4 can index into an array of length at most 7. This is
fine for the largest cpl array (which does have length 7), but is
too large for some of the others, which are smaller. This adds bounds
checks for each array access to ensure they are all within bounds.

Change-Id: I1610d35ca6cbb6cfb42c251e75b0e8b22b64252b
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229676
---
M src/southbridge/amd/sr5650/pcie.c
1 file changed, 22 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/33458/1

diff --git a/src/southbridge/amd/sr5650/pcie.c b/src/southbridge/amd/sr5650/pcie.c
index f87fadb..e90a997 100644
--- a/src/southbridge/amd/sr5650/pcie.c
+++ b/src/southbridge/amd/sr5650/pcie.c
@@ -360,41 +360,53 @@
 static void gpp3a_cpl_buf_alloc(struct device *nb_dev, struct device *dev)
 {
    u8 dev_index;
-	u8 *slave_cpl;
    u8 value;
    struct southbridge_amd_sr5650_config *cfg =
        (struct southbridge_amd_sr5650_config *)nb_dev->chip_info;

dev_index = dev->path.pci.devfn >> 3;
-	if (dev_index < 4 || dev_index > 0xa) {
+
+	if (dev_index < 4)
    	return;
-	}
+
+	dev_index -= 4;

switch (cfg->gpp3a_configuration) {
    case 0x1: /* 4:2:0:0:0:0 */
-		slave_cpl = (u8 *)&pGpp420000;
+		if (dev_index >= ARRAY_SIZE(pGpp420000))
+			return;
+		value = pGpp420000[dev_index];
    	break;
    case 0x2: /* 4:1:1:0:0:0 */
-		slave_cpl = (u8 *)&pGpp411000;
+		if (dev_index >= ARRAY_SIZE(pGpp411000))
+			return;
+		value = pGpp411000[dev_index];
    	break;
    case 0xC: /* 2:2:2:0:0:0 */
-		slave_cpl = (u8 *)&pGpp222000;
+		if (dev_index >= ARRAY_SIZE(pGpp222000))
+			return;
+		value = pGpp222000[dev_index];
    	break;
    case 0xA: /* 2:2:1:1:0:0 */
-		slave_cpl = (u8 *)&pGpp221100;
+		if (dev_index >= ARRAY_SIZE(pGpp221100))
+			return;
+		value = pGpp221100[dev_index];
    	break;
    case 0x4: /* 2:1:1:1:1:0 */
-		slave_cpl = (u8 *)&pGpp211110;
+		if (dev_index >= ARRAY_SIZE(pGpp211110))
+			return;
+		value = pGpp211110[dev_index];
    	break;
    case 0xB: /* 1:1:1:1:1:1 */
-		slave_cpl = (u8 *)&pGpp111111;
+		if (dev_index >= ARRAY_SIZE(pGpp111111))
+			return;
+		value = pGpp111111[dev_index];
    	break;
    default:  /* shouldn't be here. */
    	printk(BIOS_WARNING, "buggy gpp3a_configuration\n");
    	return;
    }

-	value = slave_cpl[dev_index - 4];
    if (value != 0) {
    	set_pcie_enable_bits(dev, 0x10, 0x3f << 8, value << 8);
    	set_pcie_enable_bits(dev, 0x20, 1 << 11, 1 << 11);

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33458/1/src/southbridge/amd/sr5650/pcie.c 
File src/southbridge/amd/sr5650/pcie.c:

https://review.coreboot.org/#/c/33458/1/src/southbridge/amd/sr5650/pcie.c@a3... 
PS1, Line 369: 
if ( dev_index !∈ ]4 ; 10 [ ) then return

now what we have is

if ( dev_index  < 4) then return

or I'm wrong

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33458/1/src/southbridge/amd/sr5650/pcie.c 
File src/southbridge/amd/sr5650/pcie.c:

https://review.coreboot.org/#/c/33458/1/src/southbridge/amd/sr5650/pcie.c@a3... 
PS1, Line 369:
> if ( dev_index !∈ ]4 ; 10 [ ) then return  […]

No that's right. After subtraction we have ]0, 6[, which means we can index into an array of length 7 (which is the length of the largest CPL array). But, each array now has its own dedicated bounds checking, which replaces the > 6 or equivalent > 10 check.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: sb/amd/sr5650: Add fine-grained bounds checking

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1: Code-Review+2

Change in coreboot[master]: sb/amd/sr5650: Add fine-grained bounds checking

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33458/1/src/southbridge/amd/sr5650/... 
File src/southbridge/amd/sr5650/pcie.c:

https://review.coreboot.org/c/coreboot/+/33458/1/src/southbridge/amd/sr5650/... 
PS1, Line 369:
> No that's right. […]

Done

Change in coreboot[master]: sb/amd/sr5650: Add fine-grained bounds checking

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33458 )

Change subject: sb/amd/sr5650: Add fine-grained bounds checking
......................................................................

sb/amd/sr5650: Add fine-grained bounds checking

The code currently checks that 4 <= dev_index <= 10, which after
subtraction by 4 can index into an array of length at most 7. This is
fine for the largest cpl array (which does have length 7), but is
too large for some of the others, which are smaller. This adds bounds
checks for each array access to ensure they are all within bounds.

Change-Id: I1610d35ca6cbb6cfb42c251e75b0e8b22b64252b
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229676
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33458
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Angel Pons 
---
M src/southbridge/amd/sr5650/pcie.c
1 file changed, 22 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/southbridge/amd/sr5650/pcie.c b/src/southbridge/amd/sr5650/pcie.c
index 6c42fdd..5c3aee7 100644
--- a/src/southbridge/amd/sr5650/pcie.c
+++ b/src/southbridge/amd/sr5650/pcie.c
@@ -360,41 +360,53 @@
 static void gpp3a_cpl_buf_alloc(struct device *nb_dev, struct device *dev)
 {
    u8 dev_index;
-	u8 *slave_cpl;
    u8 value;
    struct southbridge_amd_sr5650_config *cfg =
        (struct southbridge_amd_sr5650_config *)nb_dev->chip_info;

dev_index = dev->path.pci.devfn >> 3;
-	if (dev_index < 4 || dev_index > 0xa) {
+
+	if (dev_index < 4)
    	return;
-	}
+
+	dev_index -= 4;

switch (cfg->gpp3a_configuration) {
    case 0x1: /* 4:2:0:0:0:0 */
-		slave_cpl = (u8 *)&pGpp420000;
+		if (dev_index >= ARRAY_SIZE(pGpp420000))
+			return;
+		value = pGpp420000[dev_index];
    	break;
    case 0x2: /* 4:1:1:0:0:0 */
-		slave_cpl = (u8 *)&pGpp411000;
+		if (dev_index >= ARRAY_SIZE(pGpp411000))
+			return;
+		value = pGpp411000[dev_index];
    	break;
    case 0xC: /* 2:2:2:0:0:0 */
-		slave_cpl = (u8 *)&pGpp222000;
+		if (dev_index >= ARRAY_SIZE(pGpp222000))
+			return;
+		value = pGpp222000[dev_index];
    	break;
    case 0xA: /* 2:2:1:1:0:0 */
-		slave_cpl = (u8 *)&pGpp221100;
+		if (dev_index >= ARRAY_SIZE(pGpp221100))
+			return;
+		value = pGpp221100[dev_index];
    	break;
    case 0x4: /* 2:1:1:1:1:0 */
-		slave_cpl = (u8 *)&pGpp211110;
+		if (dev_index >= ARRAY_SIZE(pGpp211110))
+			return;
+		value = pGpp211110[dev_index];
    	break;
    case 0xB: /* 1:1:1:1:1:1 */
-		slave_cpl = (u8 *)&pGpp111111;
+		if (dev_index >= ARRAY_SIZE(pGpp111111))
+			return;
+		value = pGpp111111[dev_index];
    	break;
    default:  /* shouldn't be here. */
    	printk(BIOS_WARNING, "buggy gpp3a_configuration\n");
    	return;
    }

-	value = slave_cpl[dev_index - 4];
    if (value != 0) {
    	set_pcie_enable_bits(dev, 0x10, 0x3f << 8, value << 8);
    	set_pcie_enable_bits(dev, 0x20, 1 << 11, 1 << 11);