skb is an optional parameter, so it may be NULL.
Add check defore dereference in eth_hdr.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
Signed-off-by: Anastasia Belova <[email protected]>
---
net/core/flow_dissector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 272f09251343..05db3a8aa771 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
rcu_read_unlock();
}
- if (dissector_uses_key(flow_dissector,
+ if (skb && dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
struct ethhdr *eth = eth_hdr(skb);
struct flow_dissector_key_eth_addrs *key_eth_addrs;
--
2.30.2
Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>skb is an optional parameter, so it may be NULL.
>Add check defore dereference in eth_hdr.
>
>Found by Linux Verification Center (linuxtesting.org) with SVACE.
Either drop this line which provides no value, or attach a link to the
actual report.
>
>Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
This looks incorrect. I believe that this is the offending commit:
commit 690e36e726d00d2528bc569809048adf61550d80
Author: David S. Miller <[email protected]>
Date: Sat Aug 23 12:13:41 2014 -0700
net: Allow raw buffers to be passed into the flow dissector.
>Signed-off-by: Anastasia Belova <[email protected]>
>---
> net/core/flow_dissector.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 272f09251343..05db3a8aa771 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
> rcu_read_unlock();
> }
>
>- if (dissector_uses_key(flow_dissector,
>+ if (skb && dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> struct ethhdr *eth = eth_hdr(skb);
> struct flow_dissector_key_eth_addrs *key_eth_addrs;
Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
pw-bot: cr
>--
>2.30.2
>
On Wed, Mar 20, 2024 at 2:38 PM Jiri Pirko <[email protected]> wrote:
>
> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
> >skb is an optional parameter, so it may be NULL.
> >Add check defore dereference in eth_hdr.
> >
> >Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Either drop this line which provides no value, or attach a link to the
> actual report.
>
>
> >
> >Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>
> This looks incorrect. I believe that this is the offending commit:
> commit 690e36e726d00d2528bc569809048adf61550d80
> Author: David S. Miller <[email protected]>
> Date: Sat Aug 23 12:13:41 2014 -0700
>
> net: Allow raw buffers to be passed into the flow dissector.
>
>
>
> >Signed-off-by: Anastasia Belova <[email protected]>
> >---
> > net/core/flow_dissector.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >index 272f09251343..05db3a8aa771 100644
> >--- a/net/core/flow_dissector.c
> >+++ b/net/core/flow_dissector.c
> >@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
> > rcu_read_unlock();
> > }
> >
> >- if (dissector_uses_key(flow_dissector,
> >+ if (skb && dissector_uses_key(flow_dissector,
> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > struct ethhdr *eth = eth_hdr(skb);
> > struct flow_dissector_key_eth_addrs *key_eth_addrs;
>
> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>
It would be nice knowing in which context we could have a NULL skb and
FLOW_DISSECTOR_KEY_ETH_ADDRS
at the same time.
It seems this fix is based on some kind of static analysis, but no real bug.
Wed, Mar 20, 2024 at 02:43:22PM CET, [email protected] wrote:
>On Wed, Mar 20, 2024 at 2:38 PM Jiri Pirko <[email protected]> wrote:
>>
>> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>> >skb is an optional parameter, so it may be NULL.
>> >Add check defore dereference in eth_hdr.
>> >
>> >Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Either drop this line which provides no value, or attach a link to the
>> actual report.
>>
>>
>> >
>> >Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>>
>> This looks incorrect. I believe that this is the offending commit:
>> commit 690e36e726d00d2528bc569809048adf61550d80
>> Author: David S. Miller <[email protected]>
>> Date: Sat Aug 23 12:13:41 2014 -0700
>>
>> net: Allow raw buffers to be passed into the flow dissector.
>>
>>
>>
>> >Signed-off-by: Anastasia Belova <[email protected]>
>> >---
>> > net/core/flow_dissector.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> >index 272f09251343..05db3a8aa771 100644
>> >--- a/net/core/flow_dissector.c
>> >+++ b/net/core/flow_dissector.c
>> >@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>> > rcu_read_unlock();
>> > }
>> >
>> >- if (dissector_uses_key(flow_dissector,
>> >+ if (skb && dissector_uses_key(flow_dissector,
>> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>> > struct ethhdr *eth = eth_hdr(skb);
>> > struct flow_dissector_key_eth_addrs *key_eth_addrs;
>>
>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>>
>
>It would be nice knowing in which context we could have a NULL skb and
>FLOW_DISSECTOR_KEY_ETH_ADDRS
>at the same time.
>
>It seems this fix is based on some kind of static analysis, but no real bug.
Yeah, I agree. That's the main reason I asked for the link to the report.
20/03/24 16:38, Jiri Pirko пишет:
> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>> skb is an optional parameter, so it may be NULL.
>> Add check defore dereference in eth_hdr.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> Either drop this line which provides no value, or attach a link to the
> actual report.
>
It is an established practice for our project. You can find 700+ applied
patches with similar line:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org
>> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
> This looks incorrect. I believe that this is the offending commit:
> commit 690e36e726d00d2528bc569809048adf61550d80
> Author: David S. Miller <[email protected]>
> Date: Sat Aug 23 12:13:41 2014 -0700
>
> net: Allow raw buffers to be passed into the flow dissector.
>
Got it.
>
>> Signed-off-by: Anastasia Belova <[email protected]>
>> ---
>> net/core/flow_dissector.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 272f09251343..05db3a8aa771 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>> rcu_read_unlock();
>> }
>>
>> - if (dissector_uses_key(flow_dissector,
>> + if (skb && dissector_uses_key(flow_dissector,
>> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>> struct ethhdr *eth = eth_hdr(skb);
>> struct flow_dissector_key_eth_addrs *key_eth_addrs;
> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
I agree, I'll send the second version.
Anastasia Belova
Thu, Mar 21, 2024 at 10:36:53AM CET, [email protected] wrote:
>
>
>20/03/24 16:38, Jiri Pirko пишет:
>> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>> > skb is an optional parameter, so it may be NULL.
>> > Add check defore dereference in eth_hdr.
>> >
>> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>> Either drop this line which provides no value, or attach a link to the
>> actual report.
>>
>
>It is an established practice for our project. You can find 700+ applied
>patches with similar line:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org
Okay. So would it be possible to attach a link to the actual report?
>
>
>> > Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>> This looks incorrect. I believe that this is the offending commit:
>> commit 690e36e726d00d2528bc569809048adf61550d80
>> Author: David S. Miller <[email protected]>
>> Date: Sat Aug 23 12:13:41 2014 -0700
>>
>> net: Allow raw buffers to be passed into the flow dissector.
>>
>
>Got it.
>
>>
>> > Signed-off-by: Anastasia Belova <[email protected]>
>> > ---
>> > net/core/flow_dissector.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> > index 272f09251343..05db3a8aa771 100644
>> > --- a/net/core/flow_dissector.c
>> > +++ b/net/core/flow_dissector.c
>> > @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>> > rcu_read_unlock();
>> > }
>> >
>> > - if (dissector_uses_key(flow_dissector,
>> > + if (skb && dissector_uses_key(flow_dissector,
>> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>> > struct ethhdr *eth = eth_hdr(skb);
>> > struct flow_dissector_key_eth_addrs *key_eth_addrs;
>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>
>I agree, I'll send the second version.
>
>Anastasia Belova
>
On 3/21/24 13:57, Jiri Pirko wrote:
> Thu, Mar 21, 2024 at 10:36:53AM CET, [email protected] wrote:
>>
>>
>> 20/03/24 16:38, Jiri Pirko пишет:
>>> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>>>> skb is an optional parameter, so it may be NULL.
>>>> Add check defore dereference in eth_hdr.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>> Either drop this line which provides no value, or attach a link to the
>>> actual report.
>>>
>>
>> It is an established practice for our project. You can find 700+ applied
>> patches with similar line:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org
>
> Okay. So would it be possible to attach a link to the actual report?
>
>>
>>
>>>> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>>> This looks incorrect. I believe that this is the offending commit:
>>> commit 690e36e726d00d2528bc569809048adf61550d80
>>> Author: David S. Miller <[email protected]>
>>> Date: Sat Aug 23 12:13:41 2014 -0700
>>>
>>> net: Allow raw buffers to be passed into the flow dissector.
>>>
>>
>> Got it.
Looks like it's a static checker, there is no actual bug report or kernel oops/crash
>>
>>>
>>>> Signed-off-by: Anastasia Belova <[email protected]>
>>>> ---
>>>> net/core/flow_dissector.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>> index 272f09251343..05db3a8aa771 100644
>>>> --- a/net/core/flow_dissector.c
>>>> +++ b/net/core/flow_dissector.c
>>>> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>>>> rcu_read_unlock();
>>>> }
>>>>
>>>> - if (dissector_uses_key(flow_dissector,
>>>> + if (skb && dissector_uses_key(flow_dissector,
>>>> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>>>> struct ethhdr *eth = eth_hdr(skb);
>>>> struct flow_dissector_key_eth_addrs *key_eth_addrs;
>>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
>>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>>
>> I agree, I'll send the second version.
>>
>> Anastasia Belova
>>
>
21/03/24 13:57, Jiri Pirko пишет:
> Thu, Mar 21, 2024 at 10:36:53AM CET, [email protected] wrote:
>>
>> 20/03/24 16:38, Jiri Pirko пишет:
>>> Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>>>> skb is an optional parameter, so it may be NULL.
>>>> Add check defore dereference in eth_hdr.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>> Either drop this line which provides no value, or attach a link to the
>>> actual report.
>>>
>> It is an established practice for our project. You can find 700+ applied
>> patches with similar line:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org
> Okay. So would it be possible to attach a link to the actual report?
Unfortunately no as far as results of the SVACE static analysis tool are
not available publicly at the moment.
Also I agree that this is quite a minor fix, but I still insist
that it would be better to add a check.
>
>>
>>>> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>>> This looks incorrect. I believe that this is the offending commit:
>>> commit 690e36e726d00d2528bc569809048adf61550d80
>>> Author: David S. Miller <[email protected]>
>>> Date: Sat Aug 23 12:13:41 2014 -0700
>>>
>>> net: Allow raw buffers to be passed into the flow dissector.
>>>
>> Got it.
>>
>>>> Signed-off-by: Anastasia Belova <[email protected]>
>>>> ---
>>>> net/core/flow_dissector.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>> index 272f09251343..05db3a8aa771 100644
>>>> --- a/net/core/flow_dissector.c
>>>> +++ b/net/core/flow_dissector.c
>>>> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>>>> rcu_read_unlock();
>>>> }
>>>>
>>>> - if (dissector_uses_key(flow_dissector,
>>>> + if (skb && dissector_uses_key(flow_dissector,
>>>> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>>>> struct ethhdr *eth = eth_hdr(skb);
>>>> struct flow_dissector_key_eth_addrs *key_eth_addrs;
>>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
>>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>> I agree, I'll send the second version.
>>
>> Anastasia Belova
>>
skb is an optional parameter, so it may be NULL.
Add check defore dereference in eth_hdr.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
Signed-off-by: Anastasia Belova <[email protected]>
---
net/core/flow_dissector.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 272f09251343..68a8228ffae3 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
if (dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+ if (!skb)
+ goto out_bad;
struct ethhdr *eth = eth_hdr(skb);
struct flow_dissector_key_eth_addrs *key_eth_addrs;
--
2.30.2
Thu, Mar 21, 2024 at 01:04:22PM CET, [email protected] wrote:
>
>
>21/03/24 13:57, Jiri Pirko пишет:
>> Thu, Mar 21, 2024 at 10:36:53AM CET, [email protected] wrote:
>> >
>> > 20/03/24 16:38, Jiri Pirko пишет:
>> > > Wed, Mar 20, 2024 at 01:56:35PM CET, [email protected] wrote:
>> > > > skb is an optional parameter, so it may be NULL.
>> > > > Add check defore dereference in eth_hdr.
>> > > >
>> > > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>> > > Either drop this line which provides no value, or attach a link to the
>> > > actual report.
>> > >
>> > It is an established practice for our project. You can find 700+ applied
>> > patches with similar line:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org
>> Okay. So would it be possible to attach a link to the actual report?
>
>Unfortunately no as far as results of the SVACE static analysis tool are
>not available publicly at the moment.
So, don't mention it, has no value what so ever. No place for
advertisements like this.
>
>
>Also I agree that this is quite a minor fix, but I still insist
>that it would be better to add a check.
It is not possible (prove us wrong) to hit this bug in real world.
No point to fix nobug.
>
>>
>> >
>> > > > Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
>> > > This looks incorrect. I believe that this is the offending commit:
>> > > commit 690e36e726d00d2528bc569809048adf61550d80
>> > > Author: David S. Miller <[email protected]>
>> > > Date: Sat Aug 23 12:13:41 2014 -0700
>> > >
>> > > net: Allow raw buffers to be passed into the flow dissector.
>> > >
>> > Got it.
>> >
>> > > > Signed-off-by: Anastasia Belova <[email protected]>
>> > > > ---
>> > > > net/core/flow_dissector.c | 2 +-
>> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> > > > index 272f09251343..05db3a8aa771 100644
>> > > > --- a/net/core/flow_dissector.c
>> > > > +++ b/net/core/flow_dissector.c
>> > > > @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net,
>> > > > rcu_read_unlock();
>> > > > }
>> > > >
>> > > > - if (dissector_uses_key(flow_dissector,
>> > > > + if (skb && dissector_uses_key(flow_dissector,
>> > > > FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>> > > > struct ethhdr *eth = eth_hdr(skb);
>> > > > struct flow_dissector_key_eth_addrs *key_eth_addrs;
>> > > Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the
>> > > FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no?
>> > I agree, I'll send the second version.
>> >
>> > Anastasia Belova
>> >
>
>
On 3/21/24 15:34, Anastasia Belova wrote:
> skb is an optional parameter, so it may be NULL.
> Add check defore dereference in eth_hdr.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
> Signed-off-by: Anastasia Belova <[email protected]>
As request in the previous email please show the actual data flow that leads to a null pointer
dereference.
Also please read function description:
..
* @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
..
> ---
> net/core/flow_dissector.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 272f09251343..68a8228ffae3 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
>
> if (dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> + if (!skb)
> + goto out_bad;
> struct ethhdr *eth = eth_hdr(skb);
> struct flow_dissector_key_eth_addrs *key_eth_addrs;
>
Thu, Mar 21, 2024 at 01:34:46PM CET, [email protected] wrote:
>skb is an optional parameter, so it may be NULL.
>Add check defore dereference in eth_hdr.
>
>Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
>Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
>Signed-off-by: Anastasia Belova <[email protected]>
>---
> net/core/flow_dissector.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 272f09251343..68a8228ffae3 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
>
> if (dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>+ if (!skb)
>+ goto out_bad;
Please read my recent reply to v1.
pw-bot: cr
> struct ethhdr *eth = eth_hdr(skb);
> struct flow_dissector_key_eth_addrs *key_eth_addrs;
>
>--
>2.30.2
>
On Thu, Mar 21, 2024 at 1:35 PM Anastasia Belova <[email protected]> wrote:
>
> skb is an optional parameter, so it may be NULL.
> Add check defore dereference in eth_hdr.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
> Signed-off-by: Anastasia Belova <[email protected]>
> ---
> net/core/flow_dissector.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 272f09251343..68a8228ffae3 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
>
> if (dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> + if (!skb)
> + goto out_bad;
> struct ethhdr *eth = eth_hdr(skb);
> struct flow_dissector_key_eth_addrs *key_eth_addrs;
>
I think you ignored my prior feedback.
In which case can we go to this point with skb == NULL ?
How come nobody complained of crashes here ?
I think we need to know if adding code here is useful or not.
You have to understand that a patch like this might need days of work
from various teams in the world,
flooded by questionable CVE.
On Thu, Mar 21, 2024 at 06:16:30PM +0100, Eric Dumazet wrote:
> On Thu, Mar 21, 2024 at 1:35 PM Anastasia Belova <[email protected]> wrote:
> >
> > skb is an optional parameter, so it may be NULL.
> > Add check defore dereference in eth_hdr.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
> > Signed-off-by: Anastasia Belova <[email protected]>
> > ---
> > net/core/flow_dissector.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 272f09251343..68a8228ffae3 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
> >
> > if (dissector_uses_key(flow_dissector,
> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > + if (!skb)
> > + goto out_bad;
> > struct ethhdr *eth = eth_hdr(skb);
> > struct flow_dissector_key_eth_addrs *key_eth_addrs;
> >
>
>
> I think you ignored my prior feedback.
>
> In which case can we go to this point with skb == NULL ?
> How come nobody complained of crashes here ?
>
> I think we need to know if adding code here is useful or not.
>
> You have to understand that a patch like this might need days of work
> from various teams in the world,
> flooded by questionable CVE.
Hi Eric and Anastasia,
I have conducted a review of the callers of __skb_flow_dissect()
that I could find in net-next and my conclusion is that, given
current usage, the code path above will not be hit with a NULL skb.
A summary of the analysis is as follows.
bond_flow_dissect:
- Analysis: skb parameter may be NULL but FLOW_DISSECTOR_KEY_ETH_ADDRS
is not included in flow_keys_bonding_keys
- Conclusion: Code path in question is not hit for this user
skb_flow_dissect:
skb_flow_dissect_flow_keys:
fib6_rules_early_flow_dissect:
fib4_rules_early_flow_dissect:
__skb_get_hash_symmetric:
- Analysis: data parameter is NULL, which means that skb must be non-NULL
else a crash would occur in the following code near the top of
__skb_flow_dissect().
if (!data) {
data = skb->data;
- Conclusion: Calling eth_hdr(skb) is safe for these users
Assuming my analysis is correct (please check!) then
as this code is in the fast path for many users I think it is best
not to add this unnecessary check (which I assume is Eric's concern too).