[PATCH 10/18] brcmsmac: change strncpy+truncation to strlcpy

[PATCH 10/18] brcmsmac: change strncpy+truncation to strlcpy

[Dominique Martinet]

Generated by scripts/coccinelle/misc/strncpy_truncation.cocci

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch

 drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
index b7df576bb84d..58ccd72d672c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
@@ -584,8 +584,7 @@ struct dma_pub *dma_attach(char *name, struct brcms_c_info *wlc,
 		      rxextheadroom, nrxpost, rxoffset, txregbase, rxregbase);
 
 	/* make a private copy of our callers name */
-	strncpy(di->name, name, MAXNAMEL);
-	di->name[MAXNAMEL - 1] = '\0';
+	strlcpy(di->name, name, MAXNAMEL);
 
 	di->dmadev = core->dma_dev;
 
-- 
2.17.1

Re: [PATCH 10/18] brcmsmac: change strncpy+truncation to strlcpy

[Arend van Spriel]

On 7/13/2018 3:25 AM, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch

I would prefer to have the motivation in the commit message of this patch.

Regards,
Arend

[PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Dominique Martinet]

Generated by scripts/coccinelle/misc/strncpy_truncation.cocci

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch

 drivers/net/wireless/ti/wl1251/acx.c  | 9 +--------
 drivers/net/wireless/ti/wl18xx/main.c | 5 +----
 drivers/net/wireless/ti/wlcore/boot.c | 5 +----
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
index f78fc3880423..c4f1a63300bb 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -150,14 +150,7 @@ int wl1251_acx_fw_version(struct wl1251 *wl, char *buf, size_t len)
 	}
 
 	/* be careful with the buffer sizes */
-	strncpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
-
-	/*
-	 * if the firmware version string is exactly
-	 * sizeof(rev->fw_version) long or fw_len is less than
-	 * sizeof(rev->fw_version) it won't be null terminated
-	 */
-	buf[min(len, sizeof(rev->fw_version)) - 1] = '\0';
+	strlcpy(buf, rev->fw_version, min(len, sizeof(rev->fw_version)));
 
 out:
 	kfree(rev);
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index ca0f936fc119..8595e9bf1cfa 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -1529,12 +1529,9 @@ static int wl18xx_handle_static_data(struct wl1271 *wl,
 	struct wl18xx_static_data_priv *static_data_priv =
 		(struct wl18xx_static_data_priv *) static_data->priv;
 
-	strncpy(wl->chip.phy_fw_ver_str, static_data_priv->phy_version,
+	strlcpy(wl->chip.phy_fw_ver_str, static_data_priv->phy_version,
 		sizeof(wl->chip.phy_fw_ver_str));
 
-	/* make sure the string is NULL-terminated */
-	wl->chip.phy_fw_ver_str[sizeof(wl->chip.phy_fw_ver_str) - 1] = '\0';
-
 	wl1271_info("PHY firmware version: %s", static_data_priv->phy_version);
 
 	return 0;
diff --git a/drivers/net/wireless/ti/wlcore/boot.c b/drivers/net/wireless/ti/wlcore/boot.c
index f00509ea8aca..6b33951d5b34 100644
--- a/drivers/net/wireless/ti/wlcore/boot.c
+++ b/drivers/net/wireless/ti/wlcore/boot.c
@@ -55,12 +55,9 @@ static int wlcore_boot_parse_fw_ver(struct wl1271 *wl,
 {
 	int ret;
 
-	strncpy(wl->chip.fw_ver_str, static_data->fw_version,
+	strlcpy(wl->chip.fw_ver_str, static_data->fw_version,
 		sizeof(wl->chip.fw_ver_str));
 
-	/* make sure the string is NULL-terminated */
-	wl->chip.fw_ver_str[sizeof(wl->chip.fw_ver_str) - 1] = '\0';
-
 	ret = sscanf(wl->chip.fw_ver_str + 4, "%u.%u.%u.%u.%u",
 		     &wl->chip.fw_ver[0], &wl->chip.fw_ver[1],
 		     &wl->chip.fw_ver[2], &wl->chip.fw_ver[3],
-- 
2.17.1

Re: [PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

I don't know about other maintainers, but I know I wouldn't take such a
horrid changelog description as this :)

good luck!

greg k-h

Re: [PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Arend van Spriel]

On 7/13/2018 9:38 AM, Greg Kroah-Hartman wrote:
> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>>
>> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)
>
> good luck!

especially as that script is not in the kernel tree yet. The patch 
adding that script contains a good motivation, but I would want to see 
that in commit message of every patch or at least the gist of it.

Regards,
Arend

Re: [PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Dominique Martinet]

Arend van Spriel wrote on Fri, Jul 13, 2018:
> The patch adding that script contains a good motivation, but I would want to
> see that in commit message of every patch or at least the gist of
> it.

In retrospect, I definitely agree - I was happy I got coccinelle to work
and a bit too tired to make rationale decisions when I sent the serie as
it's not a kind of thing I'm used to.

For the patch you ack'd, in particular, there would be no gcc warning in
the first place because the source string's size is not known at compile
time and for some reason gcc does not mind silent truncation in that
case, so the usefulnes of the patch is fairly limited in the first
place (it's possibly simpler/good to aim for consistency but that's
about it). I however didn't take the time to make that analysis for all
the patches.

> especially as that script is not in the kernel tree yet.

I did think about that, but wasn't sure what was appropriate in this
case.
I now think it would have been better to save everyone a dozen of mails
and wait for the coccinelle patch to land first; but it's a bit late for
regret :)
I'll only catter after the coccinelle script until it lands, so if
anyone is inclined to take one of the rest as they are, great, but
otherwise feel free to ignore them for now.
(In particular, this very patch should not remove the first comment
here, as pointed out by Himanshu Jha in reply to the first patch)


Thanks for taking the time to give feedback,
-- 
Dominique Martinet

Re: [PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Jul 13, 2018, at 12:38 AM, Greg Kroah-Hartman  
<gregkh@linuxfoundation.org> wrote:

> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>>
>> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)
>
> good luck!
>
> greg k-h

I would be very concerned about the potential for information leak, because  
of the different behavior of the two functions.

--
Mark Rustad, Networking Division, Intel Corporation

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 11/18] wireless/ti: change strncpy+truncation to strlcpy

[Kalle Valo]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Fri, Jul 13, 2018 at 03:25:49AM +0200, Dominique Martinet wrote:
>> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>> 
>> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
>
> I don't know about other maintainers, but I know I wouldn't take such a
> horrid changelog description as this :)

I agree and I'll drop this. If this is still needed please resend with a
better commit log.

-- 
Kalle Valo