brcmfmac: fix "board_type" in brcmf_of_probe()

PATCH In review Dan Carpenter 4 participants 7 messages Jun 21, 2021 – Sep 9, 2021

Subsystems (2)

Kernel janitors Broadcom BRCM80211 IEEE802.11 wireless drivers

lore

Share & tools

lore

Dan Carpenter

author

2021-06-21 12:33 UTC #0 lore raw

PATCH v2 1/1 brcmfmac: fix "board_type" in brcmf_of_probe()

There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:

Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {

Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.

Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Basically a completely different patch. :)

 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +4 -8

index a7554265f95f..8ca62cd0e8ac 100644
@@ -24,20 +24,16 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 
 		/* get rid of '/' in the compatible string to be able to find the FW */
-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
-		}
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type)
+			return;
+		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
 
 		of_node_put(root);

Signature · 1 line

2.30.2

Hans de Goede

2021-06-21 12:41 UTC #1 lore raw

Hi,

Quoted 36 lines · Dan Carpenter

There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:
Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {
Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.
Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")
Suggested-by Johannes Berg <[email protected]>
Suggested-by Christophe JAILLET <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Basically a completely different patch. :)
 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +0 -1
index a7554265f95f..8ca62cd0e8ac 100644
@@ -24,20 +24,16 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 
 		/* get rid of '/' in the compatible string to be able to find the FW */

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, · line 35

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +3 -7

-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
-		}
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type)
+			return;

The error handling here seems to be lacking, specifically this error-exit path needs a of_node_put(root); call (or a "goto put;" or some such)

Regards,

Hans

Quoted 5 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +1 -0

+		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
 
 		of_node_put(root);

Dan Carpenter

author

2021-06-21 12:47 UTC #2 lore raw

Quoted 38 lines · Hans de Goede

Hi,
Quoted 35 lines · Dan Carpenter
There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:
Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {
Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.
Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")
Suggested-by Johannes Berg <[email protected]>
Suggested-by Christophe JAILLET <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Basically a completely different patch. :)
 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +0 -1
index a7554265f95f..8ca62cd0e8ac 100644
@@ -24,20 +24,16 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 

Quoted 15 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, · line 35
Quoted 11 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +3 -7
 		/* get rid of '/' in the compatible string to be able to find the FW */
-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
-		}
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type)
+			return;
The error handling here seems to be lacking, specifically this error-exit path needs a of_node_put(root); call (or a "goto put;" or some such)

Good catch. I will add a call to of_node_put().

regards,
dan carpenter

Hans de Goede

Reviewed-by on v2 1/1

2021-06-22 10:48 UTC #3 lore raw

In reply to Dan Carpenter

Hi,

Quoted 11 lines · Dan Carpenter

There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:
Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {
Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.

Quoted 5 lines

Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")
Suggested-by Johannes Berg <[email protected]>
Suggested-by Christophe JAILLET <[email protected]>
Reported-by Hans deGoede <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

Thanks, patch looks good to me:

Reviewed-by Hans de Goede <[email protected]>

Regards,

Hans

Quoted 38 lines

---
v2: Basically a completely different patch. :)
v3: Add missing of_node_put() caught by Hans de Goede

 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +5 -7

index a7554265f95f..dee3d968e49b 100644
@@ -24,20 +24,18 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 
 		/* get rid of '/' in the compatible string to be able to find the FW */
-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type) {
+			of_node_put(root);
+			return;
 		}
+		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
 
 		of_node_put(root);

Matthias Brugger

Reviewed-by on v2 1/1

2021-06-22 10:56 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 11 lines · Dan Carpenter

There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:
Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {
Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.

Quoted 5 lines

Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")
Suggested-by Johannes Berg <[email protected]>
Suggested-by Christophe JAILLET <[email protected]>
Reported-by Hans deGoede <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

Thanks for taking care of this.

Reviewed-by Matthias Brugger <[email protected]>

Quoted 38 lines

---
v2: Basically a completely different patch. :)
v3: Add missing of_node_put() caught by Hans de Goede

 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +5 -7

index a7554265f95f..dee3d968e49b 100644
@@ -24,20 +24,18 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 
 		/* get rid of '/' in the compatible string to be able to find the FW */
-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type) {
+			of_node_put(root);
+			return;
 		}
+		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
 
 		of_node_put(root);

Dan Carpenter

author

2021-09-08 13:13 UTC #5 lore raw

In reply to Dan Carpenter

This patch is still needed.

regards,
dan carpenter

Quoted 55 lines · Dan Carpenter

There are two bugs here:
1) devm_kzalloc() needs to be checked for allocation errors.
2) The loop was intended to be:
Bad: for (i = 0; i < board_type[i]; i++) {
Good: for (i = 0; i < len; i++) {
Neither of these bugs are likely to cause an issue in practice but they're worth fixing. Also the code could be made simpler by using the devm_kstrdup() and strreplace() functions.
Fixes 29e354ebeeec ("brcmfmac: Transform compatible string for FW loading")
Suggested-by Johannes Berg <[email protected]>
Suggested-by Christophe JAILLET <[email protected]>
Reported-by Hans deGoede <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Basically a completely different patch. :)
v3: Add missing of_node_put() caught by Hans de Goede
 .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c +5 -7
index a7554265f95f..dee3d968e49b 100644
@@ -24,20 +24,18 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
 	/* Set board-type to the first string of the machine compatible prop */
 	root = of_find_node_by_path("/");
 	if (root) {
-		int i, len;
 		char *board_type;
 		const char *tmp;
 
 		of_property_read_string_index(root, "compatible", 0, &tmp);
 
 		/* get rid of '/' in the compatible string to be able to find the FW */
-		len = strlen(tmp) + 1;
-		board_type = devm_kzalloc(dev, len, GFP_KERNEL);
-		strscpy(board_type, tmp, len);
-		for (i = 0; i < board_type[i]; i++) {
-			if (board_type[i] == '/')
-				board_type[i] = '-';
+		board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+		if (!board_type) {
+			of_node_put(root);
+			return;
 		}
+		strreplace(board_type, '/', '-');
 		settings->board_type = board_type;
 
 		of_node_put(root);
Signature · 1 line
2.30.2

Kalle Valo

2021-09-09 09:42 UTC #6 lore raw

Quoted 1 line · Dan Carpenter

This patch is still needed.

I don't see v3 in patchwork[1], but I do see v2. I don't know what happened, but please resend the patch as v4.

[1] https://patchwork.kernel.org/project/linux-wireless/list/?series=&submitter=37111&state=*&q=&archive=&delegate=

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches