[v2] net/nfp: fix possible buffer overflow
Checks
Commit Message
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
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.
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
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
>
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.
@@ -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) {