diff mbox series

[1/3] ethdev: fix null pointer checking

Message ID 20190403160726.1231-1-mohammad.abdul.awal@intel.com (mailing list archive)
State Rejected, archived
Headers
Series [1/3] ethdev: fix null pointer checking |

Checks

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

Commit Message

Mohammad Abdul Awal April 3, 2019, 4:07 p.m. UTC 
  Null value for parameter name will cause segfault for the
strnlen and strcmp functions.

Fixes: 0b33b68d12 ("ethdev: export allocate function")
Fixes: 942661004c ("ethdev: export secondary attach function")
Cc: stable@dpdk.org

Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Thomas Monjalon April 3, 2019, 4:27 p.m. UTC | #1 
03/04/2019 18:07, Mohammad Abdul Awal:
> Null value for parameter name will cause segfault for the
> strnlen and strcmp functions.

I'm not sure we want such obvious checks for all APIs.
Here I would say yes.

> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> Fixes: 942661004c ("ethdev: export secondary attach function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> ---
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
>  	struct rte_eth_dev *eth_dev = NULL;
>  	size_t name_len;
>  
> +	if (name == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");

This is a very generic error message.
It might be "Fail to allocate port with empty name"

> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char *name)
>  	uint16_t i;
>  	struct rte_eth_dev *eth_dev = NULL;
>  
> +	if (name == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");

"Fail to attach port with empty name"
Ferruh Yigit April 3, 2019, 4:35 p.m. UTC | #2 
On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> 03/04/2019 18:07, Mohammad Abdul Awal:
>> Null value for parameter name will cause segfault for the
>> strnlen and strcmp functions.
> 
> I'm not sure we want such obvious checks for all APIs.
> Here I would say yes.

These are internal functions, not APIs.
I am for verifying input for (all) APIs but not for internal functions, drivers
should call them and they are in our control, if they are passing NULL we can
fix them :)

> 
>> Fixes: 0b33b68d12 ("ethdev: export allocate function")
>> Fixes: 942661004c ("ethdev: export secondary attach function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
>> ---
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
>>  	struct rte_eth_dev *eth_dev = NULL;
>>  	size_t name_len;
>>  
>> +	if (name == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> 
> This is a very generic error message.
> It might be "Fail to allocate port with empty name"
> 
>> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char *name)
>>  	uint16_t i;
>>  	struct rte_eth_dev *eth_dev = NULL;
>>  
>> +	if (name == NULL) {
>> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> 
> "Fail to attach port with empty name"
> 
> 
>
Bruce Richardson April 3, 2019, 4:41 p.m. UTC | #3 
On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > 03/04/2019 18:07, Mohammad Abdul Awal:
> >> Null value for parameter name will cause segfault for the strnlen and
> >> strcmp functions.
> > 
> > I'm not sure we want such obvious checks for all APIs.  Here I would
> > say yes.
> 
> These are internal functions, not APIs.  I am for verifying input for
> (all) APIs but not for internal functions, drivers should call them and
> they are in our control, if they are passing NULL we can fix them :)
> 
True, but if these are control path or init time code paths rather than
data path APIs, I don't see the harm in putting in the checks.

/Bruce
Ferruh Yigit April 3, 2019, 4:52 p.m. UTC | #4 
On 4/3/2019 5:41 PM, Bruce Richardson wrote:
> On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
>> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
>>> 03/04/2019 18:07, Mohammad Abdul Awal:
>>>> Null value for parameter name will cause segfault for the strnlen and
>>>> strcmp functions.
>>>
>>> I'm not sure we want such obvious checks for all APIs.  Here I would
>>> say yes.
>>
>> These are internal functions, not APIs.  I am for verifying input for
>> (all) APIs but not for internal functions, drivers should call them and
>> they are in our control, if they are passing NULL we can fix them :)
>>
> True, but if these are control path or init time code paths rather than
> data path APIs, I don't see the harm in putting in the checks.

No harm from performance point of view, agree, but also looks unnecessary to me.
Mohammad Abdul Awal April 3, 2019, 5:30 p.m. UTC | #5 
The null checks are for the input param "char *name" of APIs rte_eth_dev_allocate and rte_eth_dev_attach_secondary.
I will change the err msg to suggested one.


> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, April 3, 2019 5:35 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Awal, Mohammad Abdul
> <mohammad.abdul.awal@intel.com>
> Cc: dev@dpdk.org; arybchenko@solarflare.com; stable@dpdk.org
> Subject: Re: [PATCH 1/3] ethdev: fix null pointer checking
> 
> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > 03/04/2019 18:07, Mohammad Abdul Awal:
> >> Null value for parameter name will cause segfault for the
> >> strnlen and strcmp functions.
> >
> > I'm not sure we want such obvious checks for all APIs.
> > Here I would say yes.
> 
> These are internal functions, not APIs.
> I am for verifying input for (all) APIs but not for internal functions, drivers
> should call them and they are in our control, if they are passing NULL we can
> fix them :)
> 
> >
> >> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> >> Fixes: 942661004c ("ethdev: export secondary attach function")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Mohammad Abdul Awal
> <mohammad.abdul.awal@intel.com>
> >> ---
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
> >>  	struct rte_eth_dev *eth_dev = NULL;
> >>  	size_t name_len;
> >>
> >> +	if (name == NULL) {
> >> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> >
> > This is a very generic error message.
> > It might be "Fail to allocate port with empty name"
> >
> >> @@ -492,6 +497,11 @@ rte_eth_dev_attach_secondary(const char
> *name)
> >>  	uint16_t i;
> >>  	struct rte_eth_dev *eth_dev = NULL;
> >>
> >> +	if (name == NULL) {
> >> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> >
> > "Fail to attach port with empty name"
> >
> >
> >
David Marchand April 3, 2019, 5:32 p.m. UTC | #6 
On Wed, Apr 3, 2019 at 6:53 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/3/2019 5:41 PM, Bruce Richardson wrote:
> > On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
> >> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> >>> 03/04/2019 18:07, Mohammad Abdul Awal:
> >>>> Null value for parameter name will cause segfault for the strnlen and
> >>>> strcmp functions.
> >>>
> >>> I'm not sure we want such obvious checks for all APIs.  Here I would
> >>> say yes.
> >>
> >> These are internal functions, not APIs.  I am for verifying input for
> >> (all) APIs but not for internal functions, drivers should call them and
> >> they are in our control, if they are passing NULL we can fix them :)
> >>
> > True, but if these are control path or init time code paths rather than
> > data path APIs, I don't see the harm in putting in the checks.
>
> No harm from performance point of view, agree, but also looks unnecessary
> to me.
>

+1
All the more when you see the following patches that adds input checks in
the faulty/too naive drivers.
Thomas Monjalon April 3, 2019, 6:42 p.m. UTC | #7 
03/04/2019 19:30, Awal, Mohammad Abdul:
> From: Yigit, Ferruh
> > On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
> > > 03/04/2019 18:07, Mohammad Abdul Awal:
> > >> Null value for parameter name will cause segfault for the
> > >> strnlen and strcmp functions.
> > >
> > > I'm not sure we want such obvious checks for all APIs.
> > > Here I would say yes.
> > 
> > These are internal functions, not APIs.
> > I am for verifying input for (all) APIs but not for internal functions, drivers
> > should call them and they are in our control, if they are passing NULL we can
> > fix them :)
> 
> The null checks are for the input param "char *name" of APIs rte_eth_dev_allocate and rte_eth_dev_attach_secondary.
> I will change the err msg to suggested one.

As Ferruh said, these are not really API in the sense that they
are not called by the applications but only by drivers.

PS: please write replies below original text.
Stephen Hemminger April 3, 2019, 6:50 p.m. UTC | #8 
On Wed,  3 Apr 2019 17:07:26 +0100
Mohammad Abdul Awal <mohammad.abdul.awal@intel.com> wrote:

> Null value for parameter name will cause segfault for the
> strnlen and strcmp functions.
> 
> Fixes: 0b33b68d12 ("ethdev: export allocate function")
> Fixes: 942661004c ("ethdev: export secondary attach function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 10bdfb37e..26898ed08 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -440,6 +440,11 @@ rte_eth_dev_allocate(const char *name)
>  	struct rte_eth_dev *eth_dev = NULL;
>  	size_t name_len;
>  
> +	if (name == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
> +		return NULL;
> +	}
> +

Ok, but I doubt that a driver that is so buggy that it passes NULL
that it will do any proper error handling on return of NULL.
Would rather just see the application crash hard immediately.
Mohammad Abdul Awal April 4, 2019, 8:33 a.m. UTC | #9 
On 03/04/2019 18:32, David Marchand wrote:
> On Wed, Apr 3, 2019 at 6:53 PM Ferruh Yigit <ferruh.yigit@intel.com 
> <mailto:ferruh.yigit@intel.com>> wrote:
>
>     On 4/3/2019 5:41 PM, Bruce Richardson wrote:
>     > On Wed, Apr 03, 2019 at 05:35:22PM +0100, Ferruh Yigit wrote:
>     >> On 4/3/2019 5:27 PM, Thomas Monjalon wrote:
>     >>> 03/04/2019 18:07, Mohammad Abdul Awal:
>     >>>> Null value for parameter name will cause segfault for the
>     strnlen and
>     >>>> strcmp functions.
>     >>>
>     >>> I'm not sure we want such obvious checks for all APIs.  Here I
>     would
>     >>> say yes.
>     >>
>     >> These are internal functions, not APIs.  I am for verifying
>     input for
>     >> (all) APIs but not for internal functions, drivers should call
>     them and
>     >> they are in our control, if they are passing NULL we can fix
>     them :)
>     >>
>     > True, but if these are control path or init time code paths
>     rather than
>     > data path APIs, I don't see the harm in putting in the checks.
>
>     No harm from performance point of view, agree, but also looks
>     unnecessary to me.
>
>
> +1
> All the more when you see the following patches that adds input checks 
> in the faulty/too naive drivers.
>
>
> -- 
> David Marchand


Self-NACK to the patch considering the discussion above.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 10bdfb37e..26898ed08 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -440,6 +440,11 @@  rte_eth_dev_allocate(const char *name)
 	struct rte_eth_dev *eth_dev = NULL;
 	size_t name_len;
 
+	if (name == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
+		return NULL;
+	}
+
 	name_len = strnlen(name, RTE_ETH_NAME_MAX_LEN);
 	if (name_len == 0) {
 		RTE_ETHDEV_LOG(ERR, "Zero length Ethernet device name\n");
@@ -492,6 +497,11 @@  rte_eth_dev_attach_secondary(const char *name)
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
+	if (name == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
+		return NULL;
+	}
+
 	rte_eth_dev_shared_data_prepare();
 
 	/* Synchronize port attachment to primary port creation and release. */