Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
Signed-off-by: Dominique Martinet <[email protected]>
---
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
Greg Kroah-Hartman <[email protected]> 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 <[email protected]>
>
> 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
On Jul 13, 2018, at 12:38 AM, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
>
> 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
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
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 <[email protected]>
>
> 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
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 <[email protected]>
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