phy: ocelot-serdes: fix out-of-bounds bug

PATCH Applied Gustavo A. R. Silva 4 participants 16 messages Oct 8, 2018 – Nov 12, 2018

Subsystems (2)

Device tree bindings Generic PHY framework

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-10-08 22:20 UTC #0 lore raw

cover phy: ocelot-serdes: fix out-of-bounds bug

This patchset aims to fix an out-of-bounds bug in
the phy-ocelot-serdes driver.

Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.

Quentin Schulz pointed out that SERDES_MAX is a valid value to index ctrl->phys. So, I updated SERDES_MAX to be SERDES6G_MAX + 1 in include/dt-bindings/phy/phy-ocelot-serdes.h.

Then I changed the condition in the for loop from
i <= SERDES_MAX to i < SERDES_MAX in order to
complete the fix.

The reason I'm sending this fix as series is because checkpatch reported an error when I first tried to integrate the whole solution into a singe patch. So, changes to dt-bindings should be sent as a separate patch.

Thanks

Gustavo A. R. Silva (2):
dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1
phy: ocelot-serdes: fix out-of-bounds read

 drivers/phy/mscc/phy-ocelot-serdes.c        | 4 ++--
 include/dt-bindings/phy/phy-ocelot-serdes.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Signature · 1 line

2.7.4

Gustavo A. R. Silva

author

2018-10-08 22:21 UTC #1 lore raw

PATCH 1/2 dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

SERDES_MAX is a valid value to index ctrl->phys in drivers/phy/mscc/phy-ocelot-serdes.c. But, currently, there is an out-of-bounds bug in the mentioned driver when reading from ctrl->phys, because the size of array ctrl->phys is SERDES_MAX.

Partially fix this by updating SERDES_MAX to be SERDES6G_MAX + 1.

Notice that this is the first part of the solution to the out-of-bounds bug mentioned above. Although this change is not dependent on any other one.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 include/dt-bindings/phy/phy-ocelot-serdes.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/dt-bindings/phy/phy-ocelot-serdes.h +1 -1

index bd28f21..fe70ada 100644
@@ -7,6 +7,6 @@
 #define SERDES1G_MAX	SERDES1G(5)
 #define SERDES6G(x)	(SERDES1G_MAX + 1 + (x))
 #define SERDES6G_MAX	SERDES6G(2)
-#define SERDES_MAX	SERDES6G_MAX
+#define SERDES_MAX	(SERDES6G_MAX + 1)
 
 #endif

Signature · 1 line

2.7.4

Quentin Schulz

Reviewed-by on v1 1/2

2018-10-09 07:27 UTC #2 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

Hi Gustavo,

Quoted 6 lines · Gustavo A. R. Silva

SERDES_MAX is a valid value to index ctrl->phys in drivers/phy/mscc/phy-ocelot-serdes.c. But, currently, there is an out-of-bounds bug in the mentioned driver when reading from ctrl->phys, because the size of array ctrl->phys is SERDES_MAX.

Quoted 6 lines

Partially fix this by updating SERDES_MAX to be SERDES6G_MAX + 1.
Notice that this is the first part of the solution to the out-of-bounds bug mentioned above. Although this change is not dependent on any other one.

Reviewed-by Quentin Schulz <[email protected]>

Thanks,
Quentin

Gustavo A. R. Silva

author

2018-10-16 08:44 UTC #3 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

Hi,

Quoted 9 lines · Quentin Schulz

Hi Gustavo,
Quoted 6 lines · Gustavo A. R. Silva
SERDES_MAX is a valid value to index ctrl->phys in drivers/phy/mscc/phy-ocelot-serdes.c. But, currently, there is an out-of-bounds bug in the mentioned driver when reading from ctrl->phys, because the size of array ctrl->phys is SERDES_MAX.

Quoted 9 lines

Quoted 6 lines
Partially fix this by updating SERDES_MAX to be SERDES6G_MAX + 1.
Notice that this is the first part of the solution to the out-of-bounds bug mentioned above. Although this change is not dependent on any other one.
Reviewed-by Quentin Schulz <[email protected]>

Friendly ping. Who can you take this?

Thanks!
--
Gustavo

Rob Herring

maintainer Applied

2018-10-17 15:09 UTC #4 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

Quoted 19 lines · Gustavo A. R. Silva

Hi,
Quoted 16 lines · Quentin Schulz
Hi Gustavo,
Quoted 12 lines · Gustavo A. R. Silva
SERDES_MAX is a valid value to index ctrl->phys in drivers/phy/mscc/phy-ocelot-serdes.c. But, currently, there is an out-of-bounds bug in the mentioned driver when reading from ctrl->phys, because the size of array ctrl->phys is SERDES_MAX.
Partially fix this by updating SERDES_MAX to be SERDES6G_MAX + 1.
Notice that this is the first part of the solution to the out-of-bounds bug mentioned above. Although this change is not dependent on any other one.

Quoted 4 lines

Quoted 2 lines
Reviewed-by Quentin Schulz <[email protected]>
Friendly ping. Who can you take this?

Applied. No need (nor benefit) to ping me. You can check the status of DT patches on patchwork[1]. If it is there and in the "New" state, it is in my queue.

Rob

[1] https://patchwork.ozlabs.org/project/devicetree-bindings/list/

Gustavo A. R. Silva

author

2018-10-17 15:19 UTC #5 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

Quoted 7 lines · Rob Herring

Quoted 2 lines
Friendly ping. Who can you take this?
Applied. No need (nor benefit) to ping me. You can check the status of DT patches on patchwork[1]. If it is there and in the "New" state, it is in my queue.

OK. I've got it. I just didn't know who usually takes these dt-bindings patches.

Quoted 4 lines

Rob
[1] https://patchwork.ozlabs.org/project/devicetree-bindings/list/

Thanks, Rob.
--
Gustavo

Rob Herring

maintainer Acked-by

2018-10-17 15:23 UTC #6 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

In reply to Rob Herring

Quoted 20 lines · Rob Herring

Quoted 19 lines · Gustavo A. R. Silva
Hi,
Quoted 16 lines · Quentin Schulz
Hi Gustavo,
Quoted 12 lines · Gustavo A. R. Silva
SERDES_MAX is a valid value to index ctrl->phys in drivers/phy/mscc/phy-ocelot-serdes.c. But, currently, there is an out-of-bounds bug in the mentioned driver when reading from ctrl->phys, because the size of array ctrl->phys is SERDES_MAX.
Partially fix this by updating SERDES_MAX to be SERDES6G_MAX + 1.
Notice that this is the first part of the solution to the out-of-bounds bug mentioned above. Although this change is not dependent on any other one.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Reviewed-by Quentin Schulz <[email protected]>
Friendly ping. Who can you take this?
Applied. No need (nor benefit) to ping me. You can check the status of DT patches on patchwork[1]. If it is there and in the "New" state, it is in my queue.

Actually, this doesn't apply to my tree as the file doesn't exist. It needs to go thru the phy tree. You didn't Cc the maintainer nor list, so resend.

Acked-by Rob Herring <[email protected]>

Rob

Gustavo A. R. Silva

author

2018-10-17 15:45 UTC #7 lore raw

Re: [PATCH 1/2] dt-bindings: phy: Update SERDES_MAX to be SERDES_MAX + 1

Quoted 10 lines · Rob Herring

Quoted 5 lines
Quoted 1 line
Friendly ping. Who can you take this?
Applied. No need (nor benefit) to ping me. You can check the status of DT patches on patchwork[1]. If it is there and in the "New" state, it is in my queue.
Actually, this doesn't apply to my tree as the file doesn't exist. It needs to go thru the phy tree. You didn't Cc the maintainer nor list, so resend.

This is what I get when I run the get_maintainer script:

linux$ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback include/dt-bindings/phy/phy-ocelot-serdes.h Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Mark Rutland <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list)

But I'll send it to the phy guys as you suggest.

Quoted 2 lines

Acked-by Rob Herring <[email protected]>

Thanks
--
Gustavo

Gustavo A. R. Silva

author

2018-10-08 22:22 UTC #8 lore raw

PATCH 2/2 phy: ocelot-serdes: fix out-of-bounds read

In reply to Gustavo A. R. Silva

Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.

Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.

Addresses-Coverity-ID: 1473966 ("Out-of-bounds read")
Addresses-Coverity-ID: 1473959 ("Out-of-bounds read")

Fixes 51f6b410fc22 ("phy: add driver for Microsemi Ocelot SerDes muxing")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/phy/mscc/phy-ocelot-serdes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/phy/mscc/phy-ocelot-serdes.c +2 -2

index 8936abd..c4eee3a 100644
@@ -206,7 +206,7 @@ static struct phy *serdes_simple_xlate(struct device *dev,
 	port = args->args[0];
 	idx = args->args[1];
 
-	for (i = 0; i <= SERDES_MAX; i++) {
+	for (i = 0; i < SERDES_MAX; i++) {
 		struct serdes_macro *macro = phy_get_drvdata(ctrl->phys[i]);
 
 		if (idx != macro->idx)

@@ -260,7 +260,7 @@ static int serdes_probe(struct platform_device *pdev)
 	if (!ctrl->regs)
 		return -ENODEV;
 
-	for (i = 0; i <= SERDES_MAX; i++) {
+	for (i = 0; i < SERDES_MAX; i++) {
 		ret = serdes_phy_create(ctrl, i, &ctrl->phys[i]);
 		if (ret)
 			return ret;

Signature · 1 line

2.7.4

Quentin Schulz

Reviewed-by on v1 2/2

2018-10-09 07:28 UTC #9 lore raw

Re: [PATCH 2/2] phy: ocelot-serdes: fix out-of-bounds read

Hi Gustavo,

Quoted 7 lines · Gustavo A. R. Silva

Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.
Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.

Reviewed-by Quentin Schulz <[email protected]>

Thanks,
Quentin

Gustavo A. R. Silva

author

2018-10-16 08:47 UTC #10 lore raw

Re: [PATCH 2/2] phy: ocelot-serdes: fix out-of-bounds read

Hi,

Quoted 7 lines · Quentin Schulz

Hi Gustavo,
Quoted 4 lines · Gustavo A. R. Silva
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.

Quoted 6 lines

Quoted 3 lines
Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.
Reviewed-by Quentin Schulz <[email protected]>

Friendly ping. Who can you take this?

Thanks!
--
Gustavo

Kishon Vijay Abraham I

2018-10-16 08:49 UTC #11 lore raw

Re: [PATCH 2/2] phy: ocelot-serdes: fix out-of-bounds read

Hi,

Quoted 10 lines · Gustavo A. R. Silva

Hi,
Quoted 7 lines · Quentin Schulz
Hi Gustavo,
Quoted 4 lines · Gustavo A. R. Silva
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.
Reviewed-by Quentin Schulz <[email protected]>
Friendly ping. Who can you take this?

This can go during the 4.20 -rc cycle.

Thanks
Kishon

Gustavo A. R. Silva

author

2018-10-17 16:13 UTC #12 lore raw

Re: [PATCH 2/2] phy: ocelot-serdes: fix out-of-bounds read

Hi Kishon,

Quoted 17 lines · Kishon Vijay Abraham I

Hi,
Quoted 14 lines · Gustavo A. R. Silva
Hi,
Quoted 11 lines · Quentin Schulz
Hi Gustavo,
Quoted 7 lines · Gustavo A. R. Silva
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.
Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Reviewed-by Quentin Schulz <[email protected]>
Friendly ping. Who can you take this?
This can go during the 4.20 -rc cycle.

Should I resend the following patch to you, so the whole series is applied to your phy tree?

https://lore.kernel.org/patchwork/patch/997326/

Thanks
--
Gustavo

Kishon Vijay Abraham I

2018-11-12 08:28 UTC #13 lore raw

Re: [PATCH 2/2] phy: ocelot-serdes: fix out-of-bounds read

Hi,

Quoted 25 lines · Gustavo A. R. Silva

Hi Kishon,
Quoted 22 lines · Kishon Vijay Abraham I
Hi,
Quoted 18 lines · Gustavo A. R. Silva
Hi,
Quoted 7 lines · Quentin Schulz
Hi Gustavo,
Quoted 4 lines · Gustavo A. R. Silva
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.
Quoted 6 lines
Quoted 3 lines
Fix this by changing the condition in the for loop from i <= SERDES_MAX to i < SERDES_MAX.
Reviewed-by Quentin Schulz <[email protected]>
Friendly ping. Who can you take this?

Quoted 7 lines

Quoted 2 lines
This can go during the 4.20 -rc cycle.
Should I resend the following patch to you, so the whole series is applied to your phy tree?
https://lore.kernel.org/patchwork/patch/997326/

This is merged by David Miller.

Thanks
Kishon

Quentin Schulz

2018-10-09 07:29 UTC #14 lore raw

In reply to Gustavo A. R. Silva

Hi Gustavo,

Quoted 15 lines · Gustavo A. R. Silva

This patchset aims to fix an out-of-bounds bug in
the phy-ocelot-serdes driver.
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.
Quentin Schulz pointed out that SERDES_MAX is a valid value to index ctrl->phys. So, I updated SERDES_MAX to be SERDES6G_MAX + 1 in include/dt-bindings/phy/phy-ocelot-serdes.h.
Then I changed the condition in the for loop from
i <= SERDES_MAX to i < SERDES_MAX in order to
complete the fix.

Quoted 6 lines

The reason I'm sending this fix as series is because checkpatch reported an error when I first tried to integrate the whole solution into a singe patch. So, changes to dt-bindings should be sent as a separate patch.

Much appreciated, thank you!

Quentin

Gustavo A. R. Silva

author

2018-10-09 14:14 UTC #15 lore raw

Quoted 18 lines · Quentin Schulz

Hi Gustavo,
Quoted 15 lines · Gustavo A. R. Silva
This patchset aims to fix an out-of-bounds bug in
the phy-ocelot-serdes driver.
Currently, there is an out-of-bounds read on array ctrl->phys, once variable i reaches the maximum array size of SERDES_MAX in the for loop.
Quentin Schulz pointed out that SERDES_MAX is a valid value to index ctrl->phys. So, I updated SERDES_MAX to be SERDES6G_MAX + 1 in include/dt-bindings/phy/phy-ocelot-serdes.h.
Then I changed the condition in the for loop from
i <= SERDES_MAX to i < SERDES_MAX in order to
complete the fix.

Quoted 9 lines

Quoted 6 lines
The reason I'm sending this fix as series is because checkpatch reported an error when I first tried to integrate the whole solution into a singe patch. So, changes to dt-bindings should be sent as a separate patch.
Much appreciated, thank you!

Glad to help. :)

Thanks
--
Gustavo