Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Tom Herbert]

On Sat, Oct 22, 2016 at 8:57 AM, Eric Garver  wrote:
> On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
>> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
>> >
>> > Can you explain why "dissector_uses_key(flow_dissector,
>> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
>> > "eth_type_vlan(proto))"?
>> >
>> > If I add uninitialized_var() here, I would at least put that in
>> > a comment here.
>>
>> Found it now myself: if skb_vlan_tag_present(skb), then we don't
>> access 'vlan', otherwise we know it is initialized because
>> eth_type_vlan(proto) has to be true.
>>
>> > On a related note, I also don't see how
>> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
>> > implies that skb is non-NULL. I guess this is related to the
>> > first one.
>>
>> I'm still unsure about this one.
>
> Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
> NULL. It uses flow_keys_buf_dissector_keys which does not specify
> FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.
>
> A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.
>
This is a serious problem. We can't rely on the callers to know which
keys they are allowed to use to avoid crashing the kernel. We should
fix those to check if skb is NULL, add a comment to the head of the
function warning people to never assume skb is non-NULL, and also
maybe add a degenerative check that both data argument and skb are not
NULL.

Tom

>> I also found something else that is suspicious: 'vlan' points
>> to the local _vlan variable, but that has gone out of scope
>> by the time we access the pointer, which doesn't seem safe.
>
> I see no harm in moving _vlan to the same scope as vlan.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Tom Herbert]

On Sat, Oct 22, 2016 at 8:57 AM, Eric Garver  wrote:
> On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
>> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
>> >
>> > Can you explain why "dissector_uses_key(flow_dissector,
>> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
>> > "eth_type_vlan(proto))"?
>> >
>> > If I add uninitialized_var() here, I would at least put that in
>> > a comment here.
>>
>> Found it now myself: if skb_vlan_tag_present(skb), then we don't
>> access 'vlan', otherwise we know it is initialized because
>> eth_type_vlan(proto) has to be true.
>>
>> > On a related note, I also don't see how
>> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
>> > implies that skb is non-NULL. I guess this is related to the
>> > first one.
>>
>> I'm still unsure about this one.
>
> Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
> NULL. It uses flow_keys_buf_dissector_keys which does not specify
> FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.
>
> A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.
>
This is a serious problem. We can't rely on the callers to know which
keys they are allowed to use to avoid crashing the kernel. We should
fix those to check if skb is NULL, add a comment to the head of the
function warning people to never assume skb is non-NULL, and also
maybe add a degenerative check that both data argument and skb are not
NULL.

Tom

>> I also found something else that is suspicious: 'vlan' points
>> to the local _vlan variable, but that has gone out of scope
>> by the time we access the pointer, which doesn't seem safe.
>
> I see no harm in moving _vlan to the same scope as vlan.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Eric Garver]

On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> > 
> > Can you explain why "dissector_uses_key(flow_dissector,
> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> > "eth_type_vlan(proto))"?
> > 
> > If I add uninitialized_var() here, I would at least put that in
> > a comment here.
> 
> Found it now myself: if skb_vlan_tag_present(skb), then we don't
> access 'vlan', otherwise we know it is initialized because
> eth_type_vlan(proto) has to be true.
>  
> > On a related note, I also don't see how
> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> > implies that skb is non-NULL. I guess this is related to the
> > first one.
> 
> I'm still unsure about this one.

Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
NULL. It uses flow_keys_buf_dissector_keys which does not specify
FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.

A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.

> I also found something else that is suspicious: 'vlan' points
> to the local _vlan variable, but that has gone out of scope
> by the time we access the pointer, which doesn't seem safe.

I see no harm in moving _vlan to the same scope as vlan.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Eric Garver]

On Sat, Oct 22, 2016 at 12:16:29AM +0200, Arnd Bergmann wrote:
> On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> > 
> > Can you explain why "dissector_uses_key(flow_dissector,
> > FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> > "eth_type_vlan(proto))"?
> > 
> > If I add uninitialized_var() here, I would at least put that in
> > a comment here.
> 
> Found it now myself: if skb_vlan_tag_present(skb), then we don't
> access 'vlan', otherwise we know it is initialized because
> eth_type_vlan(proto) has to be true.
>  
> > On a related note, I also don't see how
> > "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> > implies that skb is non-NULL. I guess this is related to the
> > first one.
> 
> I'm still unsure about this one.

Only skb_flow_dissect_flow_keys_buf() calls this function with skb ==
NULL. It uses flow_keys_buf_dissector_keys which does not specify
FLOW_DISSECTOR_KEY_VLAN, so the if statement is false.

A similar assumption is made for FLOW_DISSECTOR_KEY_ETH_ADDRS higher up.

> I also found something else that is suspicious: 'vlan' points
> to the local _vlan variable, but that has gone out of scope
> by the time we access the pointer, which doesn't seem safe.

I see no harm in moving _vlan to the same scope as vlan.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Jiri Pirko]

Sat, Oct 22, 2016 at 03:48:48AM CEST, torva...@linux-foundation.org wrote:
>On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko  wrote:
>>
>> I don't see how vlan could be used uninitialized. But I understand that
>> this is impossible for gcc to track it. Please just use uninitialized_var()
>
>Actually, I think we should never use "uninitialized_var()" except
>possibly for arrays or structures that gcc can complain about.
>
>It's a horrible thing to use, in that it adds extra cruft to the
>source code, and then shuts up a compiler warning (even the _reliable_
>warnings from gcc).
>
>It's much better to just initialize the variable, and if gcc some day
>gets smarter and sees that it  is unnecessary and always overwritten,
>so much the better. The cost of initializing a single word is
>basically zero.

On the other hand, I would agrue that initializing a var to "some" value
that is never used might confuse the reader. He would naturally try to
understand the reason for that exact value in initialization.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Jiri Pirko]

Sat, Oct 22, 2016 at 03:48:48AM CEST, torva...@linux-foundation.org wrote:
>On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko  wrote:
>>
>> I don't see how vlan could be used uninitialized. But I understand that
>> this is impossible for gcc to track it. Please just use uninitialized_var()
>
>Actually, I think we should never use "uninitialized_var()" except
>possibly for arrays or structures that gcc can complain about.
>
>It's a horrible thing to use, in that it adds extra cruft to the
>source code, and then shuts up a compiler warning (even the _reliable_
>warnings from gcc).
>
>It's much better to just initialize the variable, and if gcc some day
>gets smarter and sees that it  is unnecessary and always overwritten,
>so much the better. The cost of initializing a single word is
>basically zero.

On the other hand, I would agrue that initializing a var to "some" value
that is never used might confuse the reader. He would naturally try to
understand the reason for that exact value in initialization.

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Linus Torvalds]

On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko  wrote:
>
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()

Actually, I think we should never use "uninitialized_var()" except
possibly for arrays or structures that gcc can complain about.

It's a horrible thing to use, in that it adds extra cruft to the
source code, and then shuts up a compiler warning (even the _reliable_
warnings from gcc).

It's much better to just initialize the variable, and if gcc some day
gets smarter and sees that it  is unnecessary and always overwritten,
so much the better. The cost of initializing a single word is
basically zero.

Linus

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Linus Torvalds]

On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko  wrote:
>
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()

Actually, I think we should never use "uninitialized_var()" except
possibly for arrays or structures that gcc can complain about.

It's a horrible thing to use, in that it adds extra cruft to the
source code, and then shuts up a compiler warning (even the _reliable_
warnings from gcc).

It's much better to just initialize the variable, and if gcc some day
gets smarter and sees that it  is unnecessary and always overwritten,
so much the better. The cost of initializing a single word is
basically zero.

Linus

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Arnd Bergmann]

On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> 
> Can you explain why "dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> "eth_type_vlan(proto))"?
> 
> If I add uninitialized_var() here, I would at least put that in
> a comment here.

Found it now myself: if skb_vlan_tag_present(skb), then we don't
access 'vlan', otherwise we know it is initialized because
eth_type_vlan(proto) has to be true.
 
> On a related note, I also don't see how
> "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> implies that skb is non-NULL. I guess this is related to the
> first one.

I'm still unsure about this one.

I also found something else that is suspicious: 'vlan' points
to the local _vlan variable, but that has gone out of scope
by the time we access the pointer, which doesn't seem safe.

Arnd

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Arnd Bergmann]

On Friday, October 21, 2016 11:05:45 PM CEST Arnd Bergmann wrote:
> 
> Can you explain why "dissector_uses_key(flow_dissector,
> FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
> "eth_type_vlan(proto))"?
> 
> If I add uninitialized_var() here, I would at least put that in
> a comment here.

Found it now myself: if skb_vlan_tag_present(skb), then we don't
access 'vlan', otherwise we know it is initialized because
eth_type_vlan(proto) has to be true.
 
> On a related note, I also don't see how
> "dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
> implies that skb is non-NULL. I guess this is related to the
> first one.

I'm still unsure about this one.

I also found something else that is suspicious: 'vlan' points
to the local _vlan variable, but that has gone out of scope
by the time we access the pointer, which doesn't seem safe.

Arnd

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Arnd Bergmann]

On Friday, October 21, 2016 6:31:18 PM CEST Jiri Pirko wrote:
> Fri, Oct 21, 2016 at 05:55:53PM CEST, a...@arndb.de wrote:
> >gcc warns about an uninitialized pointer dereference in the vlan
> >priority handling:
> >
> >net/core/flow_dissector.c: In function '__skb_flow_dissect':
> >net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in 
> >this function [-Werror=maybe-uninitialized]
> >
> >From all I can tell, this warning is about a real bug, and we
> >should not attempt look up the vlan header if there was
> >no vlan tag.
> 
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()
> 

I usually try to avoid uninitialized_var(), as making it obvious to
the compiler why something is known tends to result in more readable
source code and better object code.

Can you explain why "dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
"eth_type_vlan(proto))"?

If I add uninitialized_var() here, I would at least put that in
a comment here.

On a related note, I also don't see how
"dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
implies that skb is non-NULL. I guess this is related to the
first one.

Arnd

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Arnd Bergmann]

On Friday, October 21, 2016 6:31:18 PM CEST Jiri Pirko wrote:
> Fri, Oct 21, 2016 at 05:55:53PM CEST, a...@arndb.de wrote:
> >gcc warns about an uninitialized pointer dereference in the vlan
> >priority handling:
> >
> >net/core/flow_dissector.c: In function '__skb_flow_dissect':
> >net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in 
> >this function [-Werror=maybe-uninitialized]
> >
> >From all I can tell, this warning is about a real bug, and we
> >should not attempt look up the vlan header if there was
> >no vlan tag.
> 
> I don't see how vlan could be used uninitialized. But I understand that
> this is impossible for gcc to track it. Please just use uninitialized_var()
> 

I usually try to avoid uninitialized_var(), as making it obvious to
the compiler why something is known tends to result in more readable
source code and better object code.

Can you explain why "dissector_uses_key(flow_dissector,
FLOW_DISSECTOR_KEY_VLAN) && skb_vlan_tag_present(skb)" implies
"eth_type_vlan(proto))"?

If I add uninitialized_var() here, I would at least put that in
a comment here.

On a related note, I also don't see how
"dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLAN)"
implies that skb is non-NULL. I guess this is related to the
first one.

Arnd

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Jiri Pirko]

Fri, Oct 21, 2016 at 05:55:53PM CEST, a...@arndb.de wrote:
>gcc warns about an uninitialized pointer dereference in the vlan
>priority handling:
>
>net/core/flow_dissector.c: In function '__skb_flow_dissect':
>net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in 
>this function [-Werror=maybe-uninitialized]
>
>From all I can tell, this warning is about a real bug, and we
>should not attempt look up the vlan header if there was
>no vlan tag.

I don't see how vlan could be used uninitialized. But I understand that
this is impossible for gcc to track it. Please just use uninitialized_var()



>
>Fixes: f6a66927692e ("flow_dissector: Get vlan priority in addition to vlan 
>id")
>Signed-off-by: Arnd Bergmann 
>---
>I'm not sure about this one, please have a closer look at what
>the original code does before applying.
>---
> net/core/flow_dissector.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..dd6003bf27e1 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -245,7 +245,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>   }
>   case htons(ETH_P_8021AD):
>   case htons(ETH_P_8021Q): {
>-  const struct vlan_hdr *vlan;
>+  const struct vlan_hdr *vlan = NULL;
> 
>   if (skb && skb_vlan_tag_present(skb))
>   proto = skb->protocol;
>@@ -264,7 +264,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>   }
> 
>   skip_vlan = true;
>-  if (dissector_uses_key(flow_dissector,
>+  if (vlan && dissector_uses_key(flow_dissector,
>  FLOW_DISSECTOR_KEY_VLAN)) {
>   key_vlan = skb_flow_dissector_target(flow_dissector,
>
> FLOW_DISSECTOR_KEY_VLAN,
>-- 
>2.9.0
>

Re: [PATCH] flow_dissector: avoid uninitialized variable access

[Jiri Pirko]

Fri, Oct 21, 2016 at 05:55:53PM CEST, a...@arndb.de wrote:
>gcc warns about an uninitialized pointer dereference in the vlan
>priority handling:
>
>net/core/flow_dissector.c: In function '__skb_flow_dissect':
>net/core/flow_dissector.c:281:61: error: 'vlan' may be used uninitialized in 
>this function [-Werror=maybe-uninitialized]
>
>From all I can tell, this warning is about a real bug, and we
>should not attempt look up the vlan header if there was
>no vlan tag.

I don't see how vlan could be used uninitialized. But I understand that
this is impossible for gcc to track it. Please just use uninitialized_var()



>
>Fixes: f6a66927692e ("flow_dissector: Get vlan priority in addition to vlan 
>id")
>Signed-off-by: Arnd Bergmann 
>---
>I'm not sure about this one, please have a closer look at what
>the original code does before applying.
>---
> net/core/flow_dissector.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..dd6003bf27e1 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -245,7 +245,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>   }
>   case htons(ETH_P_8021AD):
>   case htons(ETH_P_8021Q): {
>-  const struct vlan_hdr *vlan;
>+  const struct vlan_hdr *vlan = NULL;
> 
>   if (skb && skb_vlan_tag_present(skb))
>   proto = skb->protocol;
>@@ -264,7 +264,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>   }
> 
>   skip_vlan = true;
>-  if (dissector_uses_key(flow_dissector,
>+  if (vlan && dissector_uses_key(flow_dissector,
>  FLOW_DISSECTOR_KEY_VLAN)) {
>   key_vlan = skb_flow_dissector_target(flow_dissector,
>
> FLOW_DISSECTOR_KEY_VLAN,
>-- 
>2.9.0
>