Patchwork [net-next-2.6,01/10] ethtool: prevent null pointer dereference with NTUPLE set but no set_rx_ntuple

login
register
mail settings
Submitter Alexander Duyck
Date Feb. 25, 2011, 11:32 p.m.
Message ID <20110225233244.7920.26742.stgit@gitlad.jf.intel.com>
Download mbox | patch
Permalink /patch/84595/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Alexander Duyck - Feb. 25, 2011, 11:32 p.m.
This change is meant to prevent a possible null pointer dereference if
NETIF_F_NTUPLE is defined but the set_rx_ntuple function pointer is not.

This issue appears to affect all kernels since 2.6.34.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 net/core/ethtool.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Feb. 26, 2011, 12:21 a.m.
On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote:
> This change is meant to prevent a possible null pointer dereference if
> NETIF_F_NTUPLE is defined but the set_rx_ntuple function pointer is not.

I think it would be a bug for NETIF_F_NTUPLE to be enabled on a device
that doesn't have this operation.  Are there any drivers for which this
is possible?

> This issue appears to affect all kernels since 2.6.34.

If this can actually happen, the fix should go to net-2.6 and
stable@kernel.org.  However, I think that the null deference is
impossible and this really just fixes the error code.

Ben.

> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
>  net/core/ethtool.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..4843674 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -893,6 +893,9 @@ static noinline_for_stack int ethtool_set_rx_ntuple(struct net_device *dev,
>  	struct ethtool_rx_ntuple_flow_spec_container *fsc = NULL;
>  	int ret;
>  
> +	if (!ops->set_rx_ntuple)
> +		return -EOPNOTSUPP;
> +
>  	if (!(dev->features & NETIF_F_NTUPLE))
>  		return -EINVAL;
>  
>
Alexander Duyck - Feb. 26, 2011, 12:40 a.m.
On 2/25/2011 4:21 PM, Ben Hutchings wrote:
> On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote:
>> This change is meant to prevent a possible null pointer dereference if
>> NETIF_F_NTUPLE is defined but the set_rx_ntuple function pointer is not.
>
> I think it would be a bug for NETIF_F_NTUPLE to be enabled on a device
> that doesn't have this operation.  Are there any drivers for which this
> is possible?

Currently there are no drivers where this is possible.  However I 
encountered it as a result of testing the patches further on in this set.

>> This issue appears to affect all kernels since 2.6.34.
>
> If this can actually happen, the fix should go to net-2.6 and
> stable@kernel.org.  However, I think that the null deference is
> impossible and this really just fixes the error code.
>
> Ben.

It cannot occur with any of the in-kernel drivers since they all set the 
NETIF_F_NTUPLE flag and have the function defined.  However going 
forward I would like to have the option of using the network flow 
classifier interface instead of the set_rx_ntuple interface due to the 
fact that it supports many of the features I needed.

I believe this patch should apply to net-2.6 without any changes so if 
it is better placed there I will resubmit it specifically for net-2.6 
and stable.

Thanks,

Alex

>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> ---
>>
>>   net/core/ethtool.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index c1a71bb..4843674 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -893,6 +893,9 @@ static noinline_for_stack int ethtool_set_rx_ntuple(struct net_device *dev,
>>   	struct ethtool_rx_ntuple_flow_spec_container *fsc = NULL;
>>   	int ret;
>>
>> +	if (!ops->set_rx_ntuple)
>> +		return -EOPNOTSUPP;
>> +
>>   	if (!(dev->features&  NETIF_F_NTUPLE))
>>   		return -EINVAL;
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 27, 2011, 12:07 a.m.
From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Fri, 25 Feb 2011 16:40:13 -0800

> It cannot occur with any of the in-kernel drivers since they all set
> the NETIF_F_NTUPLE flag and have the function defined.  However going
> forward I would like to have the option of using the network flow
> classifier interface instead of the set_rx_ntuple interface due to the
> fact that it supports many of the features I needed.

This still doesn't explain to me why a driver would set the feature
flag, but not actually implement the feature.

I'm not applying this patch.

When you create the situation that causes the potentially NULL
dereference, then you can use that patch to show why this seemingly
illogical situation can indeed occur.

Until then no driver causes this issue, therefore the problem does
not exist.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Duyck - Feb. 27, 2011, 2:16 a.m.
On Sat, Feb 26, 2011 at 4:07 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Fri, 25 Feb 2011 16:40:13 -0800
>
>> It cannot occur with any of the in-kernel drivers since they all set
>> the NETIF_F_NTUPLE flag and have the function defined.  However going
>> forward I would like to have the option of using the network flow
>> classifier interface instead of the set_rx_ntuple interface due to the
>> fact that it supports many of the features I needed.
>
> This still doesn't explain to me why a driver would set the feature
> flag, but not actually implement the feature.

Actually the reason I ran into this is because of the patches in the
RFC set.  Basically I was looking at moving the ntuple support in
ixgbe over to network flow classifier rules.  As such I was leaving
the ntuple flag set, but using set_rxnfc via the filter rules instead.
 If you recommend adding a new flag to do that I am fine with that.

> I'm not applying this patch.
>
> When you create the situation that causes the potentially NULL
> dereference, then you can use that patch to show why this seemingly
> illogical situation can indeed occur.
>
> Until then no driver causes this issue, therefore the problem does
> not exist.

I'll do some digging late next week to see if there are any other
means of encountering the issue and will get back to you if I find
anything.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c1a71bb..4843674 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -893,6 +893,9 @@  static noinline_for_stack int ethtool_set_rx_ntuple(struct net_device *dev,
 	struct ethtool_rx_ntuple_flow_spec_container *fsc = NULL;
 	int ret;
 
+	if (!ops->set_rx_ntuple)
+		return -EOPNOTSUPP;
+
 	if (!(dev->features & NETIF_F_NTUPLE))
 		return -EINVAL;