diff mbox series

[v2] net/nfp: fix possible buffer overflow

Message ID 1552040885-15275-1-git-send-email-pallantlax.poornima@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] net/nfp: fix possible buffer overflow |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Poornima, PallantlaX March 8, 2019, 10:28 a.m. UTC 
  sprintf function is not secure as it doesn't check the length of string.
More secure function snprintf is used.

Fixes: 896c265ef9 ("net/nfp: use new CPP interface")
Fixes: c4171b520b ("net/nfp: support PF multiport")
Cc: stable@dpdk.org

Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
---
v2: updated title as suggested.
---
 drivers/net/nfp/nfp_net.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Alejandro Lucero March 12, 2019, 9:56 a.m. UTC | #1 
On Fri, Mar 8, 2019 at 10:28 AM Pallantla Poornima <
pallantlax.poornima@intel.com> wrote:

> sprintf function is not secure as it doesn't check the length of string.
> More secure function snprintf is used.
>
> Fixes: 896c265ef9 ("net/nfp: use new CPP interface")
> Fixes: c4171b520b ("net/nfp: support PF multiport")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
> ---
> v2: updated title as suggested.
> ---
>  drivers/net/nfp/nfp_net.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index a791e95e2..f63def5ef 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -3318,9 +3318,9 @@ nfp_pf_create_dev(struct rte_pci_device *dev, int
> port, int ports,
>                 return -ENOMEM;
>
>         if (ports > 1)
> -               sprintf(port_name, "%s_port%d", dev->device.name, port);
> +               snprintf(port_name, 100, "%s_port%d", dev->device.name,
> port);
>         else
> -               sprintf(port_name, "%s", dev->device.name);
> +               strlcat(port_name, dev->device.name, 100);
>
>
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> @@ -3433,12 +3433,14 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> nfp_nsp *nsp, char *card)
>         /* Looking for firmware file in order of priority */
>
>         /* First try to find a firmware image specific for this device */
> -       sprintf(serial, "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
> +       snprintf(serial, sizeof(serial),
> +                       "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
>                 cpp->serial[0], cpp->serial[1], cpp->serial[2],
> cpp->serial[3],
>                 cpp->serial[4], cpp->serial[5], cpp->interface >> 8,
>                 cpp->interface & 0xff);
>
> -       sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
> +       snprintf(fw_name, sizeof(fw_name), "%s/%s.nffw", DEFAULT_FW_PATH,
> +                       serial);
>
>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>         fw_f = open(fw_name, O_RDONLY);
> @@ -3446,7 +3448,8 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> nfp_nsp *nsp, char *card)
>                 goto read_fw;
>
>         /* Then try the PCI name */
> -       sprintf(fw_name, "%s/pci-%s.nffw", DEFAULT_FW_PATH, dev->
> device.name);
> +       snprintf(fw_name, sizeof(fw_name), "%s/pci-%s.nffw",
> DEFAULT_FW_PATH,
> +                       dev->device.name);
>
>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>         fw_f = open(fw_name, O_RDONLY);
> @@ -3454,7 +3457,7 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> nfp_nsp *nsp, char *card)
>                 goto read_fw;
>
>         /* Finally try the card type and media */
> -       sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> +       snprintf(fw_name, sizeof(fw_name), "%s/%s", DEFAULT_FW_PATH, card);
>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>         fw_f = open(fw_name, O_RDONLY);
>         if (fw_f < 0) {
> @@ -3530,8 +3533,9 @@ nfp_fw_setup(struct rte_pci_device *dev, struct
> nfp_cpp *cpp,
>
>         PMD_DRV_LOG(INFO, "Port speed: %u", nfp_eth_table->ports[0].speed);
>
> -       sprintf(card_desc, "nic_%s_%dx%d.nffw", nfp_fw_model,
> -               nfp_eth_table->count, nfp_eth_table->ports[0].speed /
> 1000);
> +       snprintf(card_desc, sizeof(card_desc), "nic_%s_%dx%d.nffw",
> +                       nfp_fw_model, nfp_eth_table->count,
> +                       nfp_eth_table->ports[0].speed / 1000);
>
>         nsp = nfp_nsp_open(cpp);
>         if (!nsp) {
> --
> 2.17.2
>
>
I got a compilation error when applying this patch: strlcat can not be
found.

I guess this patch requires to check for system libraries versions.
Ferruh Yigit March 19, 2019, 5:43 p.m. UTC | #2 
On 3/12/2019 9:56 AM, Alejandro Lucero wrote:
> On Fri, Mar 8, 2019 at 10:28 AM Pallantla Poornima <
> pallantlax.poornima@intel.com> wrote:
> 
>> sprintf function is not secure as it doesn't check the length of string.
>> More secure function snprintf is used.
>>
>> Fixes: 896c265ef9 ("net/nfp: use new CPP interface")
>> Fixes: c4171b520b ("net/nfp: support PF multiport")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
>> ---
>> v2: updated title as suggested.
>> ---
>>  drivers/net/nfp/nfp_net.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
>> index a791e95e2..f63def5ef 100644
>> --- a/drivers/net/nfp/nfp_net.c
>> +++ b/drivers/net/nfp/nfp_net.c
>> @@ -3318,9 +3318,9 @@ nfp_pf_create_dev(struct rte_pci_device *dev, int
>> port, int ports,
>>                 return -ENOMEM;
>>
>>         if (ports > 1)
>> -               sprintf(port_name, "%s_port%d", dev->device.name, port);
>> +               snprintf(port_name, 100, "%s_port%d", dev->device.name,
>> port);
>>         else
>> -               sprintf(port_name, "%s", dev->device.name);
>> +               strlcat(port_name, dev->device.name, 100);
>>
>>
>>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>> @@ -3433,12 +3433,14 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
>> nfp_nsp *nsp, char *card)
>>         /* Looking for firmware file in order of priority */
>>
>>         /* First try to find a firmware image specific for this device */
>> -       sprintf(serial, "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
>> +       snprintf(serial, sizeof(serial),
>> +                       "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
>>                 cpp->serial[0], cpp->serial[1], cpp->serial[2],
>> cpp->serial[3],
>>                 cpp->serial[4], cpp->serial[5], cpp->interface >> 8,
>>                 cpp->interface & 0xff);
>>
>> -       sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
>> +       snprintf(fw_name, sizeof(fw_name), "%s/%s.nffw", DEFAULT_FW_PATH,
>> +                       serial);
>>
>>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>>         fw_f = open(fw_name, O_RDONLY);
>> @@ -3446,7 +3448,8 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
>> nfp_nsp *nsp, char *card)
>>                 goto read_fw;
>>
>>         /* Then try the PCI name */
>> -       sprintf(fw_name, "%s/pci-%s.nffw", DEFAULT_FW_PATH, dev->
>> device.name);
>> +       snprintf(fw_name, sizeof(fw_name), "%s/pci-%s.nffw",
>> DEFAULT_FW_PATH,
>> +                       dev->device.name);
>>
>>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>>         fw_f = open(fw_name, O_RDONLY);
>> @@ -3454,7 +3457,7 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
>> nfp_nsp *nsp, char *card)
>>                 goto read_fw;
>>
>>         /* Finally try the card type and media */
>> -       sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
>> +       snprintf(fw_name, sizeof(fw_name), "%s/%s", DEFAULT_FW_PATH, card);
>>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
>>         fw_f = open(fw_name, O_RDONLY);
>>         if (fw_f < 0) {
>> @@ -3530,8 +3533,9 @@ nfp_fw_setup(struct rte_pci_device *dev, struct
>> nfp_cpp *cpp,
>>
>>         PMD_DRV_LOG(INFO, "Port speed: %u", nfp_eth_table->ports[0].speed);
>>
>> -       sprintf(card_desc, "nic_%s_%dx%d.nffw", nfp_fw_model,
>> -               nfp_eth_table->count, nfp_eth_table->ports[0].speed /
>> 1000);
>> +       snprintf(card_desc, sizeof(card_desc), "nic_%s_%dx%d.nffw",
>> +                       nfp_fw_model, nfp_eth_table->count,
>> +                       nfp_eth_table->ports[0].speed / 1000);
>>
>>         nsp = nfp_nsp_open(cpp);
>>         if (!nsp) {
>> --
>> 2.17.2
>>
>>
> I got a compilation error when applying this patch: strlcat can not be
> found.
> 
> I guess this patch requires to check for system libraries versions.
> 

Hi Alejandro,

Linux doesn't have the 'strlcat' but there is DPDK implementation of it, comes
with '#include <rte_string_fns.h>' header which is already included in this file.

'strlcat' support is added in this release, 19.05, can you be using an old code?
Can you please double check the build with the latest code?

Thanks,
ferruh
Alejandro Lucero March 21, 2019, 10:10 a.m. UTC | #3 
On Tue, Mar 19, 2019 at 5:43 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/12/2019 9:56 AM, Alejandro Lucero wrote:
> > On Fri, Mar 8, 2019 at 10:28 AM Pallantla Poornima <
> > pallantlax.poornima@intel.com> wrote:
> >
> >> sprintf function is not secure as it doesn't check the length of string.
> >> More secure function snprintf is used.
> >>
> >> Fixes: 896c265ef9 ("net/nfp: use new CPP interface")
> >> Fixes: c4171b520b ("net/nfp: support PF multiport")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
> >> ---
> >> v2: updated title as suggested.
> >> ---
> >>  drivers/net/nfp/nfp_net.c | 20 ++++++++++++--------
> >>  1 file changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> >> index a791e95e2..f63def5ef 100644
> >> --- a/drivers/net/nfp/nfp_net.c
> >> +++ b/drivers/net/nfp/nfp_net.c
> >> @@ -3318,9 +3318,9 @@ nfp_pf_create_dev(struct rte_pci_device *dev, int
> >> port, int ports,
> >>                 return -ENOMEM;
> >>
> >>         if (ports > 1)
> >> -               sprintf(port_name, "%s_port%d", dev->device.name,
> port);
> >> +               snprintf(port_name, 100, "%s_port%d", dev->device.name,
> >> port);
> >>         else
> >> -               sprintf(port_name, "%s", dev->device.name);
> >> +               strlcat(port_name, dev->device.name, 100);
> >>
> >>
> >>         if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >> @@ -3433,12 +3433,14 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> >> nfp_nsp *nsp, char *card)
> >>         /* Looking for firmware file in order of priority */
> >>
> >>         /* First try to find a firmware image specific for this device
> */
> >> -       sprintf(serial,
> "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
> >> +       snprintf(serial, sizeof(serial),
> >> +
>  "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
> >>                 cpp->serial[0], cpp->serial[1], cpp->serial[2],
> >> cpp->serial[3],
> >>                 cpp->serial[4], cpp->serial[5], cpp->interface >> 8,
> >>                 cpp->interface & 0xff);
> >>
> >> -       sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
> >> +       snprintf(fw_name, sizeof(fw_name), "%s/%s.nffw",
> DEFAULT_FW_PATH,
> >> +                       serial);
> >>
> >>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
> >>         fw_f = open(fw_name, O_RDONLY);
> >> @@ -3446,7 +3448,8 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> >> nfp_nsp *nsp, char *card)
> >>                 goto read_fw;
> >>
> >>         /* Then try the PCI name */
> >> -       sprintf(fw_name, "%s/pci-%s.nffw", DEFAULT_FW_PATH, dev->
> >> device.name);
> >> +       snprintf(fw_name, sizeof(fw_name), "%s/pci-%s.nffw",
> >> DEFAULT_FW_PATH,
> >> +                       dev->device.name);
> >>
> >>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
> >>         fw_f = open(fw_name, O_RDONLY);
> >> @@ -3454,7 +3457,7 @@ nfp_fw_upload(struct rte_pci_device *dev, struct
> >> nfp_nsp *nsp, char *card)
> >>                 goto read_fw;
> >>
> >>         /* Finally try the card type and media */
> >> -       sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
> >> +       snprintf(fw_name, sizeof(fw_name), "%s/%s", DEFAULT_FW_PATH,
> card);
> >>         PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
> >>         fw_f = open(fw_name, O_RDONLY);
> >>         if (fw_f < 0) {
> >> @@ -3530,8 +3533,9 @@ nfp_fw_setup(struct rte_pci_device *dev, struct
> >> nfp_cpp *cpp,
> >>
> >>         PMD_DRV_LOG(INFO, "Port speed: %u",
> nfp_eth_table->ports[0].speed);
> >>
> >> -       sprintf(card_desc, "nic_%s_%dx%d.nffw", nfp_fw_model,
> >> -               nfp_eth_table->count, nfp_eth_table->ports[0].speed /
> >> 1000);
> >> +       snprintf(card_desc, sizeof(card_desc), "nic_%s_%dx%d.nffw",
> >> +                       nfp_fw_model, nfp_eth_table->count,
> >> +                       nfp_eth_table->ports[0].speed / 1000);
> >>
> >>         nsp = nfp_nsp_open(cpp);
> >>         if (!nsp) {
> >> --
> >> 2.17.2
> >>
> >>
> > I got a compilation error when applying this patch: strlcat can not be
> > found.
> >
> > I guess this patch requires to check for system libraries versions.
> >
>
> Hi Alejandro,
>
>
Hi Ferruh,


> Linux doesn't have the 'strlcat' but there is DPDK implementation of it,
> comes
> with '#include <rte_string_fns.h>' header which is already included in
> this file.
>
> 'strlcat' support is added in this release, 19.05, can you be using an old
> code?
> Can you please double check the build with the latest code?
>
>
I have tried again with tip DPDK and it works fine. I would say I used also
tip the first time, but anyway, it compiles now without problem.

I have also performed some basic tests and it is all fine.

So:

Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Thanks!


> Thanks,
> ferruh
>
Ferruh Yigit March 21, 2019, 5:03 p.m. UTC | #4 
On 3/21/2019 10:10 AM, Alejandro Lucero wrote:
> 
> 
> On Tue, Mar 19, 2019 at 5:43 PM Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 3/12/2019 9:56 AM, Alejandro Lucero wrote:
>     > On Fri, Mar 8, 2019 at 10:28 AM Pallantla Poornima <
>     > pallantlax.poornima@intel.com <mailto:pallantlax.poornima@intel.com>> wrote:
>     >
>     >> sprintf function is not secure as it doesn't check the length of string.
>     >> More secure function snprintf is used.
>     >>
>     >> Fixes: 896c265ef9 ("net/nfp: use new CPP interface")
>     >> Fixes: c4171b520b ("net/nfp: support PF multiport")
>     >> Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>     >>
>     >> Signed-off-by: Pallantla Poornima <pallantlax.poornima@intel.com>
>
> Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index a791e95e2..f63def5ef 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3318,9 +3318,9 @@  nfp_pf_create_dev(struct rte_pci_device *dev, int port, int ports,
 		return -ENOMEM;
 
 	if (ports > 1)
-		sprintf(port_name, "%s_port%d", dev->device.name, port);
+		snprintf(port_name, 100, "%s_port%d", dev->device.name, port);
 	else
-		sprintf(port_name, "%s", dev->device.name);
+		strlcat(port_name, dev->device.name, 100);
 
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
@@ -3433,12 +3433,14 @@  nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 	/* Looking for firmware file in order of priority */
 
 	/* First try to find a firmware image specific for this device */
-	sprintf(serial, "serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
+	snprintf(serial, sizeof(serial),
+			"serial-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x",
 		cpp->serial[0], cpp->serial[1], cpp->serial[2], cpp->serial[3],
 		cpp->serial[4], cpp->serial[5], cpp->interface >> 8,
 		cpp->interface & 0xff);
 
-	sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);
+	snprintf(fw_name, sizeof(fw_name), "%s/%s.nffw", DEFAULT_FW_PATH,
+			serial);
 
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
 	fw_f = open(fw_name, O_RDONLY);
@@ -3446,7 +3448,8 @@  nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 		goto read_fw;
 
 	/* Then try the PCI name */
-	sprintf(fw_name, "%s/pci-%s.nffw", DEFAULT_FW_PATH, dev->device.name);
+	snprintf(fw_name, sizeof(fw_name), "%s/pci-%s.nffw", DEFAULT_FW_PATH,
+			dev->device.name);
 
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
 	fw_f = open(fw_name, O_RDONLY);
@@ -3454,7 +3457,7 @@  nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 		goto read_fw;
 
 	/* Finally try the card type and media */
-	sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
+	snprintf(fw_name, sizeof(fw_name), "%s/%s", DEFAULT_FW_PATH, card);
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
 	fw_f = open(fw_name, O_RDONLY);
 	if (fw_f < 0) {
@@ -3530,8 +3533,9 @@  nfp_fw_setup(struct rte_pci_device *dev, struct nfp_cpp *cpp,
 
 	PMD_DRV_LOG(INFO, "Port speed: %u", nfp_eth_table->ports[0].speed);
 
-	sprintf(card_desc, "nic_%s_%dx%d.nffw", nfp_fw_model,
-		nfp_eth_table->count, nfp_eth_table->ports[0].speed / 1000);
+	snprintf(card_desc, sizeof(card_desc), "nic_%s_%dx%d.nffw",
+			nfp_fw_model, nfp_eth_table->count,
+			nfp_eth_table->ports[0].speed / 1000);
 
 	nsp = nfp_nsp_open(cpp);
 	if (!nsp) {