kern/176369: [PATCH] mxge: Correct some problem and do some cleanups
Christoph Mallon
christoph.mallon at gmx.de
Sat Feb 23 11:40:01 UTC 2013
>Number: 176369
>Category: kern
>Synopsis: [PATCH] mxge: Correct some problem and do some cleanups
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: update
>Submitter-Id: current-users
>Arrival-Date: Sat Feb 23 11:40:00 UTC 2013
>Closed-Date:
>Last-Modified:
>Originator: Christoph Mallon
>Release:
>Organization:
>Environment:
>Description:
This patch series corrects some problems (potential buffer overruns) and performs some cleanups in the mxge driver.
- Remove pointless null pointer tests after malloc(..., M_WAITOK).
- Use strncmp() instead of memcmp().
- Use strlcpy() instead of the error-prone strncpy().
- Remove the unused unused union qualhack.
- Check the MAC address in the EEPROM string more strictly.
- Expand the macro MXGE_NEXT_STRING() at its only user.
- Remove unnecessary buffer limit check.
>How-To-Repeat:
>Fix:
Please apply these patches.
--- 0001-mxge-Remove-pointless-null-pointer-tests-after-mallo.patch begins here ---
>From e65e89fece12ea9ac5b353875e366a103f71e479 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 11:06:56 +0100
Subject: [PATCH 1/7] mxge: Remove pointless null pointer tests after
malloc(..., M_WAITOK).
Some cases would have bogusly returned 0 as error code anyway.
---
sys/dev/mxge/if_mxge.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index 245c139..a8c8aa4 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -3325,8 +3325,6 @@ mxge_alloc_slice_rings(struct mxge_slice_state *ss, int rx_ring_entries,
size_t bytes;
int err, i;
- err = ENOMEM;
-
/* allocate per-slice receive resources */
ss->rx_small.mask = ss->rx_big.mask = rx_ring_entries - 1;
@@ -3335,24 +3333,16 @@ mxge_alloc_slice_rings(struct mxge_slice_state *ss, int rx_ring_entries,
/* allocate the rx shadow rings */
bytes = rx_ring_entries * sizeof (*ss->rx_small.shadow);
ss->rx_small.shadow = malloc(bytes, M_DEVBUF, M_ZERO|M_WAITOK);
- if (ss->rx_small.shadow == NULL)
- return err;
bytes = rx_ring_entries * sizeof (*ss->rx_big.shadow);
ss->rx_big.shadow = malloc(bytes, M_DEVBUF, M_ZERO|M_WAITOK);
- if (ss->rx_big.shadow == NULL)
- return err;
/* allocate the rx host info rings */
bytes = rx_ring_entries * sizeof (*ss->rx_small.info);
ss->rx_small.info = malloc(bytes, M_DEVBUF, M_ZERO|M_WAITOK);
- if (ss->rx_small.info == NULL)
- return err;
bytes = rx_ring_entries * sizeof (*ss->rx_big.info);
ss->rx_big.info = malloc(bytes, M_DEVBUF, M_ZERO|M_WAITOK);
- if (ss->rx_big.info == NULL)
- return err;
/* allocate the rx busdma resources */
err = bus_dma_tag_create(sc->parent_dmat, /* parent */
@@ -3449,8 +3439,6 @@ mxge_alloc_slice_rings(struct mxge_slice_state *ss, int rx_ring_entries,
bytes = 8 +
sizeof (*ss->tx.req_list) * (ss->tx.max_desc + 4);
ss->tx.req_bytes = malloc(bytes, M_DEVBUF, M_WAITOK);
- if (ss->tx.req_bytes == NULL)
- return err;
/* ensure req_list entries are aligned to 8 bytes */
ss->tx.req_list = (mcp_kreq_ether_send_t *)
((unsigned long)(ss->tx.req_bytes + 7) & ~7UL);
@@ -3459,14 +3447,10 @@ mxge_alloc_slice_rings(struct mxge_slice_state *ss, int rx_ring_entries,
bytes = sizeof (*ss->tx.seg_list) * ss->tx.max_desc;
ss->tx.seg_list = (bus_dma_segment_t *)
malloc(bytes, M_DEVBUF, M_WAITOK);
- if (ss->tx.seg_list == NULL)
- return err;
/* allocate the tx host info ring */
bytes = tx_ring_entries * sizeof (*ss->tx.info);
ss->tx.info = malloc(bytes, M_DEVBUF, M_ZERO|M_WAITOK);
- if (ss->tx.info == NULL)
- return err;
/* allocate the tx busdma resources */
err = bus_dma_tag_create(sc->parent_dmat, /* parent */
--
1.8.1.3
--- 0001-mxge-Remove-pointless-null-pointer-tests-after-mallo.patch ends here ---
--- dummy1 begins here ---
dummy file, because GNATS damages every other file
--- dummy1 ends here ---
--- 0002-mxge-Use-strncmp-instead-of-memcmp.patch begins here ---
>From 8187c30fc513ce4e9e57692ea9f21495fd92d7cc Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 11:13:24 +0100
Subject: [PATCH 2/7] mxge: Use strncmp() instead of memcmp().
memcmp() assumes that both buffers are at least as big as the given length.
This might not hold for ptr, but it is guaranteed to be NUL terminated.
---
sys/dev/mxge/if_mxge.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index a8c8aa4..1c1f74a 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -298,7 +298,7 @@ mxge_parse_strings(mxge_softc_t *sc)
found_mac = 0;
found_sn2 = 0;
while (ptr < limit && *ptr != '\0') {
- if (memcmp(ptr, "MAC=", 4) == 0) {
+ if (strncmp(ptr, "MAC=", 4) == 0) {
ptr += 1;
sc->mac_addr_string = ptr;
for (i = 0; i < 6; i++) {
@@ -308,15 +308,15 @@ mxge_parse_strings(mxge_softc_t *sc)
sc->mac_addr[i] = strtoul(ptr, NULL, 16);
found_mac = 1;
}
- } else if (memcmp(ptr, "PC=", 3) == 0) {
+ } else if (strncmp(ptr, "PC=", 3) == 0) {
ptr += 3;
strncpy(sc->product_code_string, ptr,
sizeof (sc->product_code_string) - 1);
- } else if (!found_sn2 && (memcmp(ptr, "SN=", 3) == 0)) {
+ } else if (!found_sn2 && (strncmp(ptr, "SN=", 3) == 0)) {
ptr += 3;
strncpy(sc->serial_number_string, ptr,
sizeof (sc->serial_number_string) - 1);
- } else if (memcmp(ptr, "SN2=", 4) == 0) {
+ } else if (strncmp(ptr, "SN2=", 4) == 0) {
/* SN2 takes precedence over SN */
ptr += 4;
found_sn2 = 1;
--
1.8.1.3
--- 0002-mxge-Use-strncmp-instead-of-memcmp.patch ends here ---
--- dummy2 begins here ---
dummy file, because GNATS damages every other file
--- dummy2 ends here ---
--- 0003-mxge-Use-strlcpy-instead-of-the-error-prone-strncpy.patch begins here ---
>From 88577d758e90542b822f93ea5a6ab53384c67629 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 11:17:59 +0100
Subject: [PATCH 3/7] mxge: Use strlcpy() instead of the error-prone strncpy().
In one case, NUL termination was not guaranteed.
---
sys/dev/mxge/if_mxge.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index 1c1f74a..a5c0a4e 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -310,18 +310,18 @@ mxge_parse_strings(mxge_softc_t *sc)
}
} else if (strncmp(ptr, "PC=", 3) == 0) {
ptr += 3;
- strncpy(sc->product_code_string, ptr,
- sizeof (sc->product_code_string) - 1);
+ strlcpy(sc->product_code_string, ptr,
+ sizeof(sc->product_code_string));
} else if (!found_sn2 && (strncmp(ptr, "SN=", 3) == 0)) {
ptr += 3;
- strncpy(sc->serial_number_string, ptr,
- sizeof (sc->serial_number_string) - 1);
+ strlcpy(sc->serial_number_string, ptr,
+ sizeof(sc->serial_number_string));
} else if (strncmp(ptr, "SN2=", 4) == 0) {
/* SN2 takes precedence over SN */
ptr += 4;
found_sn2 = 1;
- strncpy(sc->serial_number_string, ptr,
- sizeof (sc->serial_number_string) - 1);
+ strlcpy(sc->serial_number_string, ptr,
+ sizeof(sc->serial_number_string));
}
MXGE_NEXT_STRING(ptr);
}
@@ -667,7 +667,7 @@ mxge_validate_firmware(mxge_softc_t *sc, const mcp_gen_header_t *hdr)
}
/* save firmware version for sysctl */
- strncpy(sc->fw_version, hdr->version, sizeof (sc->fw_version));
+ strlcpy(sc->fw_version, hdr->version, sizeof(sc->fw_version));
if (mxge_verbose)
device_printf(sc->dev, "firmware id: %s\n", hdr->version);
--
1.8.1.3
--- 0003-mxge-Use-strlcpy-instead-of-the-error-prone-strncpy.patch ends here ---
--- dummy3 begins here ---
dummy file, because GNATS damages every other file
--- dummy3 ends here ---
--- 0004-mxge-Remove-the-unused-unused-union-qualhack.patch begins here ---
>From 4fa46cd078dfe5dadd975d322322d1c6f4f17bb6 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 11:27:22 +0100
Subject: [PATCH 4/7] mxge: Remove the unused unused union qualhack.
---
sys/dev/mxge/if_mxge.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index a5c0a4e..5158c89 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -649,12 +649,6 @@ abort:
return (mxge_load_firmware(sc, 0));
}
-union qualhack
-{
- const char *ro_char;
- char *rw_char;
-};
-
static int
mxge_validate_firmware(mxge_softc_t *sc, const mcp_gen_header_t *hdr)
{
--
1.8.1.3
--- 0004-mxge-Remove-the-unused-unused-union-qualhack.patch ends here ---
--- dummy4 begins here ---
dummy file, because GNATS damages every other file
--- dummy4 ends here ---
--- 0005-mxge-Check-the-MAC-address-in-the-EEPROM-string-more.patch begins here ---
>From b4eafb7dd4c520849278977801685c68ce1af4fa Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 12:07:47 +0100
Subject: [PATCH 5/7] mxge: Check the MAC address in the EEPROM string more
strictly.
---
sys/dev/mxge/if_mxge.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index 5158c89..ca7c7c7 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -292,6 +292,7 @@ mxge_parse_strings(mxge_softc_t *sc)
char *ptr, *limit;
int i, found_mac, found_sn2;
+ char *endptr;
ptr = sc->eeprom_strings;
limit = sc->eeprom_strings + MXGE_EEPROM_STRINGS_SIZE;
@@ -299,15 +300,18 @@ mxge_parse_strings(mxge_softc_t *sc)
found_sn2 = 0;
while (ptr < limit && *ptr != '\0') {
if (strncmp(ptr, "MAC=", 4) == 0) {
- ptr += 1;
- sc->mac_addr_string = ptr;
- for (i = 0; i < 6; i++) {
- ptr += 3;
- if ((ptr + 2) > limit)
+ ptr += 4;
+ for (i = 0;;) {
+ sc->mac_addr[i] = strtoul(ptr, &endptr, 16);
+ if (endptr - ptr != 2)
+ goto abort;
+ ptr = endptr;
+ if (++i == 6)
+ break;
+ if (*ptr++ != ':')
goto abort;
- sc->mac_addr[i] = strtoul(ptr, NULL, 16);
- found_mac = 1;
}
+ found_mac = 1;
} else if (strncmp(ptr, "PC=", 3) == 0) {
ptr += 3;
strlcpy(sc->product_code_string, ptr,
--
1.8.1.3
--- 0005-mxge-Check-the-MAC-address-in-the-EEPROM-string-more.patch ends here ---
--- dummy5 begins here ---
dummy file, because GNATS damages every other file
--- dummy5 ends here ---
--- 0006-mxge-Expand-the-macro-MXGE_NEXT_STRING-at-its-only-u.patch begins here ---
>From 058dacee6d3128366b8a45b352019328a6ab5f9c Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 12:11:38 +0100
Subject: [PATCH 6/7] mxge: Expand the macro MXGE_NEXT_STRING() at its only
user.
Due to a typo (ptr vs. p) it would not have worked anywhere else anyway.
---
sys/dev/mxge/if_mxge.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index ca7c7c7..0a90f7c 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -288,8 +288,6 @@ mxge_dma_free(mxge_dma_t *dma)
static int
mxge_parse_strings(mxge_softc_t *sc)
{
-#define MXGE_NEXT_STRING(p) while(ptr < limit && *ptr++)
-
char *ptr, *limit;
int i, found_mac, found_sn2;
char *endptr;
@@ -327,7 +325,7 @@ mxge_parse_strings(mxge_softc_t *sc)
strlcpy(sc->serial_number_string, ptr,
sizeof(sc->serial_number_string));
}
- MXGE_NEXT_STRING(ptr);
+ while (ptr < limit && *ptr++ != '\0') {}
}
if (found_mac)
--
1.8.1.3
--- 0006-mxge-Expand-the-macro-MXGE_NEXT_STRING-at-its-only-u.patch ends here ---
--- dummy6 begins here ---
dummy file, because GNATS damages every other file
--- dummy6 ends here ---
--- 0007-mxge-Remove-unnecessary-buffer-limit-check.patch begins here ---
>From 358c48a79a8a41b3e6804949d8532ef7ceedd147 Mon Sep 17 00:00:00 2001
From: Christoph Mallon <christoph.mallon at gmx.de>
Date: Sat, 23 Feb 2013 12:13:10 +0100
Subject: [PATCH 7/7] mxge: Remove unnecessary buffer limit check.
The buffer is double-NUL terminated per construction.
---
sys/dev/mxge/if_mxge.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/sys/dev/mxge/if_mxge.c b/sys/dev/mxge/if_mxge.c
index 0a90f7c..fc07f8c 100644
--- a/sys/dev/mxge/if_mxge.c
+++ b/sys/dev/mxge/if_mxge.c
@@ -288,15 +288,14 @@ mxge_dma_free(mxge_dma_t *dma)
static int
mxge_parse_strings(mxge_softc_t *sc)
{
- char *ptr, *limit;
+ char *ptr;
int i, found_mac, found_sn2;
char *endptr;
ptr = sc->eeprom_strings;
- limit = sc->eeprom_strings + MXGE_EEPROM_STRINGS_SIZE;
found_mac = 0;
found_sn2 = 0;
- while (ptr < limit && *ptr != '\0') {
+ while (*ptr != '\0') {
if (strncmp(ptr, "MAC=", 4) == 0) {
ptr += 4;
for (i = 0;;) {
@@ -325,7 +324,7 @@ mxge_parse_strings(mxge_softc_t *sc)
strlcpy(sc->serial_number_string, ptr,
sizeof(sc->serial_number_string));
}
- while (ptr < limit && *ptr++ != '\0') {}
+ while (*ptr++ != '\0') {}
}
if (found_mac)
--
1.8.1.3
--- 0007-mxge-Remove-unnecessary-buffer-limit-check.patch ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list