[dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function

[dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function

[Kirill Rybalchenko]

num parameter for strncpy() function should be smaller than
actual destination buffer size to allow null termination.

Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")

Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
---
 drivers/net/i40e/rte_pmd_i40e.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
index 4881ea0..489f66b 100644
--- a/drivers/net/i40e/rte_pmd_i40e.c
+++ b/drivers/net/i40e/rte_pmd_i40e.c
@@ -1928,7 +1928,7 @@ int rte_pmd_i40e_get_ddp_info(uint8_t *pkg_buff, uint32_t pkg_size,
 		for (i = j = 0; i < nb_rec; j++) {
 			pinfo[j].proto_id = tlv->data[0];
 			strncpy(pinfo[j].name, (const char *)&tlv->data[1],
-				I40E_DDP_NAME_SIZE);
+				I40E_DDP_NAME_SIZE - 1);
 			i += tlv->len;
 			tlv = &tlv[tlv->len];
 		}
-- 
2.5.5

Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function

[Bruce Richardson]

On Mon, Oct 23, 2017 at 12:24:49PM +0100, Kirill Rybalchenko wrote:
> num parameter for strncpy() function should be smaller than
> actual destination buffer size to allow null termination.
> 
> Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> ---
>  drivers/net/i40e/rte_pmd_i40e.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
> index 4881ea0..489f66b 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.c
> +++ b/drivers/net/i40e/rte_pmd_i40e.c
> @@ -1928,7 +1928,7 @@ int rte_pmd_i40e_get_ddp_info(uint8_t *pkg_buff, uint32_t pkg_size,
>  		for (i = j = 0; i < nb_rec; j++) {
>  			pinfo[j].proto_id = tlv->data[0];
>  			strncpy(pinfo[j].name, (const char *)&tlv->data[1],
> -				I40E_DDP_NAME_SIZE);
> +				I40E_DDP_NAME_SIZE - 1);
>  			i += tlv->len;
>  			tlv = &tlv[tlv->len];
>  		}
> -- 

This is not a proper fix, as it still won't null-terminate the result.
Replace strncpy with snprintf is probably the best solution.

/Bruce

Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function

[Rybalchenko, Kirill]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday 23 October 2017 13:58
> To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>
> Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for
> strncpy function
> 
> On Mon, Oct 23, 2017 at 12:24:49PM +0100, Kirill Rybalchenko wrote:
> > num parameter for strncpy() function should be smaller than actual
> > destination buffer size to allow null termination.
> >
> > Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")
> >
> > Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> > ---
> >  drivers/net/i40e/rte_pmd_i40e.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/rte_pmd_i40e.c
> > b/drivers/net/i40e/rte_pmd_i40e.c index 4881ea0..489f66b 100644
> > --- a/drivers/net/i40e/rte_pmd_i40e.c
> > +++ b/drivers/net/i40e/rte_pmd_i40e.c
> > @@ -1928,7 +1928,7 @@ int rte_pmd_i40e_get_ddp_info(uint8_t
> *pkg_buff, uint32_t pkg_size,
> >  		for (i = j = 0; i < nb_rec; j++) {
> >  			pinfo[j].proto_id = tlv->data[0];
> >  			strncpy(pinfo[j].name, (const char *)&tlv->data[1],
> > -				I40E_DDP_NAME_SIZE);
> > +				I40E_DDP_NAME_SIZE - 1);
> >  			i += tlv->len;
> >  			tlv = &tlv[tlv->len];
> >  		}
> > --
> 
> This is not a proper fix, as it still won't null-terminate the result.
> Replace strncpy with snprintf is probably the best solution.
The source buffer is 12 bytes long and guaranteed contains null-terminated C-string,
Destination buffer is 32 bytes long. Strncpy documentation says:
" Copies the first num characters of source to destination. If the end of the source C string (which is signaled by a null-character) is found before num characters have been copied, destination is padded with zeros until a total of num characters have been written to it."
I belive, having that, we can be sure that we'll get proper null-terminated string within destination buffer.
Or do I miss something?
> 
> /Bruce

Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for strncpy function

[Bruce Richardson]

On Mon, Oct 23, 2017 at 02:06:05PM +0100, Rybalchenko, Kirill wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday 23 October 2017 13:58
> > To: Rybalchenko, Kirill <kirill.rybalchenko@intel.com>
> > Cc: dev@dpdk.org; Chilikin, Andrey <andrey.chilikin@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix value of num parameter for
> > strncpy function
> > 
> > On Mon, Oct 23, 2017 at 12:24:49PM +0100, Kirill Rybalchenko wrote:
> > > num parameter for strncpy() function should be smaller than actual
> > > destination buffer size to allow null termination.
> > >
> > > Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")
> > >
> > > Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> > > ---
> > >  drivers/net/i40e/rte_pmd_i40e.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/i40e/rte_pmd_i40e.c
> > > b/drivers/net/i40e/rte_pmd_i40e.c index 4881ea0..489f66b 100644
> > > --- a/drivers/net/i40e/rte_pmd_i40e.c
> > > +++ b/drivers/net/i40e/rte_pmd_i40e.c
> > > @@ -1928,7 +1928,7 @@ int rte_pmd_i40e_get_ddp_info(uint8_t
> > *pkg_buff, uint32_t pkg_size,
> > >  		for (i = j = 0; i < nb_rec; j++) {
> > >  			pinfo[j].proto_id = tlv->data[0];
> > >  			strncpy(pinfo[j].name, (const char *)&tlv->data[1],
> > > -				I40E_DDP_NAME_SIZE);
> > > +				I40E_DDP_NAME_SIZE - 1);
> > >  			i += tlv->len;
> > >  			tlv = &tlv[tlv->len];
> > >  		}
> > > --
> > 
> > This is not a proper fix, as it still won't null-terminate the result.
> > Replace strncpy with snprintf is probably the best solution.
> The source buffer is 12 bytes long and guaranteed contains null-terminated C-string,
> Destination buffer is 32 bytes long. Strncpy documentation says:
> " Copies the first num characters of source to destination. If the end of the source C string (which is signaled by a null-character) is found before num characters have been copied, destination is padded with zeros until a total of num characters have been written to it."
> I belive, having that, we can be sure that we'll get proper null-terminated string within destination buffer.
> Or do I miss something?

Well, yes and no. Given what you say, the patch proposed is pointless,
given that both 32 and 31 are greater than 12.

So, either we accept the existing code as correct as-is, and that it
will always null-terminate because the destination buffer is bigger, or,
we decide that we want to guarantee that the buffer will be
null-terminated irrespective of the source and destination buffer sizes,
in which case you need to use snprintf, or manually zero the last byte
of the buffer. In either case, the patch you propose doesn't help, as
reducing the destination buffer length to strncpy *never* causes strncpy
to null-terminate where it failed to do so before.

My 2c, in the absense of a sane strcpy function like strlcpy, snprintf
should always be used in place of strncpy. Strncpy is just too error
prone.

/Bruce

[dpdk-dev] [PATCH] net/i40e: fix unsecure usage of strncpy function

[Kirill Rybalchenko]

Use more secure snprintf function instead of strncpy
to prevent memory access violation.

Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")

Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
---
 drivers/net/i40e/rte_pmd_i40e.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/rte_pmd_i40e.c b/drivers/net/i40e/rte_pmd_i40e.c
index 4881ea0..3df4dcc 100644
--- a/drivers/net/i40e/rte_pmd_i40e.c
+++ b/drivers/net/i40e/rte_pmd_i40e.c
@@ -1927,8 +1927,8 @@ int rte_pmd_i40e_get_ddp_info(uint8_t *pkg_buff, uint32_t pkg_size,
 		tlv = (struct i40e_profile_tlv_section_record *)&proto[1];
 		for (i = j = 0; i < nb_rec; j++) {
 			pinfo[j].proto_id = tlv->data[0];
-			strncpy(pinfo[j].name, (const char *)&tlv->data[1],
-				I40E_DDP_NAME_SIZE);
+			snprintf(pinfo[j].name, I40E_DDP_NAME_SIZE, "%s",
+				 (const char *)&tlv->data[1]);
 			i += tlv->len;
 			tlv = &tlv[tlv->len];
 		}
-- 
2.5.5

Re: [dpdk-dev] [PATCH] net/i40e: fix unsecure usage of strncpy function

[Bruce Richardson]

On Tue, Oct 24, 2017 at 10:22:38AM +0100, Kirill Rybalchenko wrote:
> Use more secure snprintf function instead of strncpy
> to prevent memory access violation.
> 
> Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")
> 
> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] net/i40e: fix unsecure usage of strncpy function

[Ferruh Yigit]

On 10/24/2017 2:50 AM, Bruce Richardson wrote:
> On Tue, Oct 24, 2017 at 10:22:38AM +0100, Kirill Rybalchenko wrote:
>> Use more secure snprintf function instead of strncpy
>> to prevent memory access violation.
>>
>> Fixes: 40d1324423a4 ("net/i40e: get ddp profile protocol info")
>>
>> Signed-off-by: Kirill Rybalchenko <kirill.rybalchenko@intel.com>
> 
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Applied to dpdk-next-net/master, thanks.