* [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy @ 2014-09-14 17:33 Rickard Strandqvist 2014-09-15 2:05 ` Stephen Hemminger 2014-09-15 3:19 ` Stephen Hemminger 0 siblings, 2 replies; 7+ messages in thread From: Rickard Strandqvist @ 2014-09-14 17:33 UTC (permalink / raw) To: Mirko Lindner, Stephen Hemminger Cc: Rickard Strandqvist, netdev, linux-kernel Replacing strncpy with strlcpy to avoid strings that lacks null terminate. Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> --- drivers/net/ethernet/marvell/sky2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c index dba48a5c..7053d38 100644 --- a/drivers/net/ethernet/marvell/sky2.c +++ b/drivers/net/ethernet/marvell/sky2.c @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) }; if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); else snprintf(buf, sz, "(chip %#x)", chipid); return buf; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-14 17:33 [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist @ 2014-09-15 2:05 ` Stephen Hemminger 2014-09-15 17:07 ` David Miller 2014-09-15 3:19 ` Stephen Hemminger 1 sibling, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2014-09-15 2:05 UTC (permalink / raw) To: Rickard Strandqvist; +Cc: Mirko Lindner, netdev, linux-kernel On Sun, 14 Sep 2014 19:33:43 +0200 Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: > Replacing strncpy with strlcpy to avoid strings that lacks null terminate. > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > drivers/net/ethernet/marvell/sky2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > index dba48a5c..7053d38 100644 > --- a/drivers/net/ethernet/marvell/sky2.c > +++ b/drivers/net/ethernet/marvell/sky2.c > @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > }; > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > else > snprintf(buf, sz, "(chip %#x)", chipid); > return buf; Useless and unnecessary since the list of names is right there. Why not avoid the copy all together? Subject: sky2: avoid strncpy Don't use strncpy() since security thought police think it is bad. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- a/drivers/net/ethernet/marvell/sky2.c 2014-08-25 09:01:16.292060455 -0700 +++ b/drivers/net/ethernet/marvell/sky2.c 2014-09-14 19:02:26.731034094 -0700 @@ -4889,7 +4889,7 @@ static int sky2_test_msi(struct sky2_hw } /* This driver supports yukon2 chipset only */ -static const char *sky2_name(u8 chipid, char *buf, int sz) +static const char *sky2_name(u8 chipid) { const char *name[] = { "XL", /* 0xb3 */ @@ -4905,11 +4905,12 @@ static const char *sky2_name(u8 chipid, "OptimaEEE", /* 0xbd */ "Optima 2", /* 0xbe */ }; + static char buf[16]; if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); - else - snprintf(buf, sz, "(chip %#x)", chipid); + return name[chipid - CHIP_ID_YUKON_XL]; + + snprintf(buf, sizeof(buf), "(chip %#x)", chipid); return buf; } @@ -4919,7 +4920,6 @@ static int sky2_probe(struct pci_dev *pd struct sky2_hw *hw; int err, using_dac = 0, wol_default; u32 reg; - char buf1[16]; err = pci_enable_device(pdev); if (err) { @@ -5014,7 +5014,7 @@ static int sky2_probe(struct pci_dev *pd } dev_info(&pdev->dev, "Yukon-2 %s chip revision %d\n", - sky2_name(hw->chip_id, buf1, sizeof(buf1)), hw->chip_rev); + sky2_name(hw->chip_id), hw->chip_rev); sky2_reset(hw); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-15 2:05 ` Stephen Hemminger @ 2014-09-15 17:07 ` David Miller 2014-09-15 20:53 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2014-09-15 17:07 UTC (permalink / raw) To: stephen; +Cc: rickard_strandqvist, mlindner, netdev, linux-kernel From: Stephen Hemminger <stephen@networkplumber.org> Date: Sun, 14 Sep 2014 19:05:57 -0700 > On Sun, 14 Sep 2014 19:33:43 +0200 > Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: > >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. >> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> >> --- >> drivers/net/ethernet/marvell/sky2.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c >> index dba48a5c..7053d38 100644 >> --- a/drivers/net/ethernet/marvell/sky2.c >> +++ b/drivers/net/ethernet/marvell/sky2.c >> @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) >> }; >> >> if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) >> - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >> + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >> else >> snprintf(buf, sz, "(chip %#x)", chipid); >> return buf; > > Useless and unnecessary since the list of names is right there. > Why not avoid the copy all together? > > Subject: sky2: avoid strncpy > > Don't use strncpy() since security thought police think it is bad. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> I think providing the buffer on the stack of the thread executing the probe is superior because it will allow enabling parallel probing in the future. I don't think you have to change that aspect to achieve your goal of returning the const char * string when possible. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-15 17:07 ` David Miller @ 2014-09-15 20:53 ` Stephen Hemminger 2014-09-15 20:56 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2014-09-15 20:53 UTC (permalink / raw) To: David Miller; +Cc: rickard_strandqvist, mlindner, netdev, linux-kernel On Mon, 15 Sep 2014 13:07:21 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Sun, 14 Sep 2014 19:05:57 -0700 > > > On Sun, 14 Sep 2014 19:33:43 +0200 > > Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: > > > >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. > >> > >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > >> --- > >> drivers/net/ethernet/marvell/sky2.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > >> index dba48a5c..7053d38 100644 > >> --- a/drivers/net/ethernet/marvell/sky2.c > >> +++ b/drivers/net/ethernet/marvell/sky2.c > >> @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > >> }; > >> > >> if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > >> - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > >> + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > >> else > >> snprintf(buf, sz, "(chip %#x)", chipid); > >> return buf; > > > > Useless and unnecessary since the list of names is right there. > > Why not avoid the copy all together? > > > > Subject: sky2: avoid strncpy > > > > Don't use strncpy() since security thought police think it is bad. > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > I think providing the buffer on the stack of the thread executing the > probe is superior because it will allow enabling parallel probing > in the future. > > I don't think you have to change that aspect to achieve your goal > of returning the const char * string when possible. What is benefit of s/strncpy/strlcpy/ for known safe code? Seems like more of the checkpatch police state. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-15 20:53 ` Stephen Hemminger @ 2014-09-15 20:56 ` David Miller 2014-10-14 22:11 ` Rickard Strandqvist 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2014-09-15 20:56 UTC (permalink / raw) To: stephen; +Cc: rickard_strandqvist, mlindner, netdev, linux-kernel From: Stephen Hemminger <stephen@networkplumber.org> Date: Mon, 15 Sep 2014 13:53:39 -0700 > On Mon, 15 Sep 2014 13:07:21 -0400 (EDT) > David Miller <davem@davemloft.net> wrote: > >> From: Stephen Hemminger <stephen@networkplumber.org> >> Date: Sun, 14 Sep 2014 19:05:57 -0700 >> >> > On Sun, 14 Sep 2014 19:33:43 +0200 >> > Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: >> > >> >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. >> >> >> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> >> >> --- >> >> drivers/net/ethernet/marvell/sky2.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c >> >> index dba48a5c..7053d38 100644 >> >> --- a/drivers/net/ethernet/marvell/sky2.c >> >> +++ b/drivers/net/ethernet/marvell/sky2.c >> >> @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) >> >> }; >> >> >> >> if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) >> >> - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >> >> + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >> >> else >> >> snprintf(buf, sz, "(chip %#x)", chipid); >> >> return buf; >> > >> > Useless and unnecessary since the list of names is right there. >> > Why not avoid the copy all together? >> > >> > Subject: sky2: avoid strncpy >> > >> > Don't use strncpy() since security thought police think it is bad. >> > >> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >> >> I think providing the buffer on the stack of the thread executing the >> probe is superior because it will allow enabling parallel probing >> in the future. >> >> I don't think you have to change that aspect to achieve your goal >> of returning the const char * string when possible. > > What is benefit of s/strncpy/strlcpy/ for known safe code? > Seems like more of the checkpatch police state. > Stephen, read my reply again. I didn't say to go back to the strlcpy change. I said to use your patch, but keep the buf[] parameter allocated on the caller's stack. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-15 20:56 ` David Miller @ 2014-10-14 22:11 ` Rickard Strandqvist 0 siblings, 0 replies; 7+ messages in thread From: Rickard Strandqvist @ 2014-10-14 22:11 UTC (permalink / raw) To: David Miller Cc: Stephen Hemminger, Mirko Lindner, Network Development, linux-kernel@vger.kernel.org 2014-09-15 22:56 GMT+02:00 David Miller <davem@davemloft.net>: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Mon, 15 Sep 2014 13:53:39 -0700 > >> On Mon, 15 Sep 2014 13:07:21 -0400 (EDT) >> David Miller <davem@davemloft.net> wrote: >> >>> From: Stephen Hemminger <stephen@networkplumber.org> >>> Date: Sun, 14 Sep 2014 19:05:57 -0700 >>> >>> > On Sun, 14 Sep 2014 19:33:43 +0200 >>> > Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: >>> > >>> >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate. >>> >> >>> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> >>> >> --- >>> >> drivers/net/ethernet/marvell/sky2.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c >>> >> index dba48a5c..7053d38 100644 >>> >> --- a/drivers/net/ethernet/marvell/sky2.c >>> >> +++ b/drivers/net/ethernet/marvell/sky2.c >>> >> @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) >>> >> }; >>> >> >>> >> if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) >>> >> - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >>> >> + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); >>> >> else >>> >> snprintf(buf, sz, "(chip %#x)", chipid); >>> >> return buf; >>> > >>> > Useless and unnecessary since the list of names is right there. >>> > Why not avoid the copy all together? >>> > >>> > Subject: sky2: avoid strncpy >>> > >>> > Don't use strncpy() since security thought police think it is bad. >>> > >>> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >>> >>> I think providing the buffer on the stack of the thread executing the >>> probe is superior because it will allow enabling parallel probing >>> in the future. >>> >>> I don't think you have to change that aspect to achieve your goal >>> of returning the const char * string when possible. >> >> What is benefit of s/strncpy/strlcpy/ for known safe code? >> Seems like more of the checkpatch police state. >> > > Stephen, read my reply again. > > I didn't say to go back to the strlcpy change. > > I said to use your patch, but keep the buf[] parameter allocated on > the caller's stack. > > Thanks. Hi Has not happened anything here ... Stephen, how can this be "." Then you might as well use strcpy, because the use here does not guarantee that the string will be null terminated, anyway. And David, as I understand you want to use code like this: static const char *sky2_name(u8 chipid, char *buf, int sz) { .... if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) return name[chipid - CHIP_ID_YUKON_XL]; snprintf(buf, sz, "(chip %#x)", chipid); return buf; } Or? And we can all easily see how the code is used here. So Stephen statement as true, sz = 16. And David, since we only use the return value. But is it really such we should assume always is true? What if someone uses it like this: char tmp[8]; ... sky2_name(id, tmp, sizeof(tmp)); printf("The chip: %s\n", tmp); Kind regards Rickard Strandqvist ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy 2014-09-14 17:33 [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist 2014-09-15 2:05 ` Stephen Hemminger @ 2014-09-15 3:19 ` Stephen Hemminger 1 sibling, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2014-09-15 3:19 UTC (permalink / raw) To: Rickard Strandqvist; +Cc: Mirko Lindner, netdev, linux-kernel On Sun, 14 Sep 2014 19:33:43 +0200 Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote: > Replacing strncpy with strlcpy to avoid strings that lacks null terminate. > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > drivers/net/ethernet/marvell/sky2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c > index dba48a5c..7053d38 100644 > --- a/drivers/net/ethernet/marvell/sky2.c > +++ b/drivers/net/ethernet/marvell/sky2.c > @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz) > }; > > if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2) > - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz); > else > snprintf(buf, sz, "(chip %#x)", chipid); > return buf; Useless and unnecessary since the list of names is right there. Why not avoid the copy? ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-10-14 22:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-14 17:33 [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy Rickard Strandqvist 2014-09-15 2:05 ` Stephen Hemminger 2014-09-15 17:07 ` David Miller 2014-09-15 20:53 ` Stephen Hemminger 2014-09-15 20:56 ` David Miller 2014-10-14 22:11 ` Rickard Strandqvist 2014-09-15 3:19 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).