| Message ID | 20180822132215.27669-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cb18e2e9ec71d42409a51b83546686c609780dde |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: fix wrong strnchr usage | expand |
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > strnchr takes arguments in the order of its name: string, max bytes to > read, character to search for. Here we're passing '\n' aka 10 as the > buffer size, and searching for sizeof(buf) aka BRCMF_DCMD_SMLEN aka > 256 (aka '\0', since it's implicitly converted to char) within those 10 > bytes. > > Just interchanging the last two arguments would still leave a bug, > because if we've been successful once, there are not sizeof(buf) > characters left after the new value of p. > > Since clmver is immediately afterwards passed as a %s argument, I assume > that it is actually a properly nul-terminated string. For that case, we > have strreplace(). > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Patch applied to wireless-drivers-next.git, thanks. cb18e2e9ec71 brcmfmac: fix wrong strnchr usage
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index cd3651069d0c..94044a7a6021 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -296,9 +296,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) /* Replace all newline/linefeed characters with space * character */ - ptr = clmver; - while ((ptr = strnchr(ptr, '\n', sizeof(buf))) != NULL) - *ptr = ' '; + strreplace(clmver, '\n', ' '); brcmf_dbg(INFO, "CLM version = %s\n", clmver); }
strnchr takes arguments in the order of its name: string, max bytes to read, character to search for. Here we're passing '\n' aka 10 as the buffer size, and searching for sizeof(buf) aka BRCMF_DCMD_SMLEN aka 256 (aka '\0', since it's implicitly converted to char) within those 10 bytes. Just interchanging the last two arguments would still leave a bug, because if we've been successful once, there are not sizeof(buf) characters left after the new value of p. Since clmver is immediately afterwards passed as a %s argument, I assume that it is actually a properly nul-terminated string. For that case, we have strreplace(). Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Incidentally, I found this because I was looking at our strnchr implementation and noticed a corner case: strchr() is specified by C and POSIX standards to consider the trailing '\0' to be part of the string, so strchr(foo, '\0') must be equivalent to "foo + strlen(foo)". strnchr() is not specified by anyone. Our implementation returns NULL as soon as count is depleted or hitting a '\0' byte, so since we were here looking for a '\0' byte, the first strnchr() call was guaranteed to return NULL. Had strnchr() instead returned a pointer to the '\0' if such was found within the first count (in this case 10) bytes, we'd be overwriting that with a ' ', and continue until no '\0' was found within the next 10 bytes. drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)