Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1506108 > unrolled thread

[PATCH] flow_dissector: avoid uninitialized variable access

Back to article view | Back to linux.kernel

Contents

  [PATCH] flow_dissector: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-10-21 18:00 +0200
    Re: [PATCH] flow_dissector: avoid uninitialized variable access Jiri Pirko <jiri@resnulli.us> - 2016-10-21 18:40 +0200
      Re: [PATCH] flow_dissector: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-10-21 23:10 +0200
        Re: [PATCH] flow_dissector: avoid uninitialized variable access Arnd Bergmann <arnd@arndb.de> - 2016-10-22 00:20 +0200
          Re: [PATCH] flow_dissector: avoid uninitialized variable access Eric Garver <e@erig.me> - 2016-10-22 18:00 +0200
            Re: [PATCH] flow_dissector: avoid uninitialized variable access Tom Herbert <tom@herbertland.com> - 2016-10-22 20:30 +0200
              [PATCH net-next] flow_dissector: fix vlan tag handling Arnd Bergmann <arnd@arndb.de> - 2016-10-23 22:00 +0200
                Re: [PATCH net-next] flow_dissector: fix vlan tag handling Jiri Pirko <jiri@resnulli.us> - 2016-10-24 10:20 +0200
                  Re: [PATCH net-next] flow_dissector: fix vlan tag handling Arnd Bergmann <arnd@arndb.de> - 2016-10-24 18:10 +0200
      Re: [PATCH] flow_dissector: avoid uninitialized variable access Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-22 03:50 +0200
        Re: [PATCH] flow_dissector: avoid uninitialized variable access Jiri Pirko <jiri@resnulli.us> - 2016-10-22 09:00 +0200

#1506108 — [PATCH] flow_dissector: avoid uninitialized variable access

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.

Fixes: f6a66927692e ("flow_dissector: Get vlan priority in addition to vlan id")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
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

[toc] | [next] | [standalone]

#1506134

Fri, Oct 21, 2016 at 05:55:53PM CEST, arnd@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 <arnd@arndb.de>
>---
>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
>

[toc] | [prev] | [next] | [standalone]

#1506288

On Friday, October 21, 2016 6:31:18 PM CEST Jiri Pirko wrote:
> Fri, Oct 21, 2016 at 05:55:53PM CEST, arnd@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

[toc] | [prev] | [next] | [standalone]

#1506330

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

[toc] | [prev] | [next] | [standalone]

#1506559

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.

[toc] | [prev] | [next] | [standalone]

#1506577

On Sat, Oct 22, 2016 at 8:57 AM, Eric Garver <e@erig.me> 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.

[toc] | [prev] | [next] | [standalone]

#1506750 — [PATCH net-next] flow_dissector: fix vlan tag handling

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]

As pointed out by Jiri Pirko, the variable is never actually used
without being initialized first as the only way it end up uninitialized
is with skb_vlan_tag_present(skb)==true, and that means it does not
get accessed.

However, the warning hints at some related issues that I'm addressing
here:

- the second check for the vlan tag is different from the first one
  that tests the skb for being NULL first, causing both the warning
  and a possible NULL pointer dereference that was not entirely fixed.
- The same patch that introduced the NULL pointer check dropped an
  earlier optimization that skipped the repeated check of the
  protocol type
- The local '_vlan' variable is referenced through the 'vlan' pointer
  but the variable has gone out of scope by the time that it is
  accessed, causing undefined behavior as the stack may have been
  overwritten.

Caching the result of the 'skb && skb_vlan_tag_present(skb)' check
in a local variable allows the compiler to further optimize the
later check. With those changes, the warning also disappears.

Fixes: 3805a938a6c2 ("flow_dissector: Check skb for VLAN only if skb specified.")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 44e6ba9d3a6b..17be1b66cc41 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	case htons(ETH_P_8021AD):
 	case htons(ETH_P_8021Q): {
 		const struct vlan_hdr *vlan;
+		struct vlan_hdr _vlan;
+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));
 
-		if (skb && skb_vlan_tag_present(skb))
-			proto = skb->protocol;
-
-		if (eth_type_vlan(proto)) {
-			struct vlan_hdr _vlan;
-
+		if (!vlan_tag_present || eth_type_vlan(skb->protocol)) {
 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
 						    data, hlen, &_vlan);
 			if (!vlan)
@@ -270,7 +267,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 							     FLOW_DISSECTOR_KEY_VLAN,
 							     target_container);
 
-			if (skb_vlan_tag_present(skb)) {
+			if (vlan_tag_present) {
 				key_vlan->vlan_id = skb_vlan_tag_get_id(skb);
 				key_vlan->vlan_priority =
 					(skb_vlan_tag_get_prio(skb) >> VLAN_PRIO_SHIFT);

[toc] | [prev] | [next] | [standalone]

#1506982 — Re: [PATCH net-next] flow_dissector: fix vlan tag handling

Sat, Oct 22, 2016 at 10:30:08PM CEST, arnd@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]
>
>As pointed out by Jiri Pirko, the variable is never actually used
>without being initialized first as the only way it end up uninitialized
>is with skb_vlan_tag_present(skb)==true, and that means it does not
>get accessed.
>
>However, the warning hints at some related issues that I'm addressing
>here:
>
>- the second check for the vlan tag is different from the first one
>  that tests the skb for being NULL first, causing both the warning
>  and a possible NULL pointer dereference that was not entirely fixed.
>- The same patch that introduced the NULL pointer check dropped an
>  earlier optimization that skipped the repeated check of the
>  protocol type
>- The local '_vlan' variable is referenced through the 'vlan' pointer
>  but the variable has gone out of scope by the time that it is
>  accessed, causing undefined behavior as the stack may have been
>  overwritten.
>
>Caching the result of the 'skb && skb_vlan_tag_present(skb)' check
>in a local variable allows the compiler to further optimize the
>later check. With those changes, the warning also disappears.
>
>Fixes: 3805a938a6c2 ("flow_dissector: Check skb for VLAN only if skb specified.")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 44e6ba9d3a6b..17be1b66cc41 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	case htons(ETH_P_8021AD):
> 	case htons(ETH_P_8021Q): {
> 		const struct vlan_hdr *vlan;
>+		struct vlan_hdr _vlan;
>+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));

Drop the unnecessary "()"


> 
>-		if (skb && skb_vlan_tag_present(skb))
>-			proto = skb->protocol;

This does not look correct. I believe that you need to set proto for
further processing.


>-
>-		if (eth_type_vlan(proto)) {
>-			struct vlan_hdr _vlan;
>-
>+		if (!vlan_tag_present || eth_type_vlan(skb->protocol)) {
> 			vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan),
> 						    data, hlen, &_vlan);
> 			if (!vlan)
>@@ -270,7 +267,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 							     FLOW_DISSECTOR_KEY_VLAN,
> 							     target_container);
> 
>-			if (skb_vlan_tag_present(skb)) {
>+			if (vlan_tag_present) {
> 				key_vlan->vlan_id = skb_vlan_tag_get_id(skb);
> 				key_vlan->vlan_priority =
> 					(skb_vlan_tag_get_prio(skb) >> VLAN_PRIO_SHIFT);
>

[toc] | [prev] | [next] | [standalone]

#1507349 — Re: [PATCH net-next] flow_dissector: fix vlan tag handling

On Monday, October 24, 2016 10:17:36 AM CEST Jiri Pirko wrote:
> Sat, Oct 22, 2016 at 10:30:08PM CEST, arnd@arndb.de wrote:
> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >index 44e6ba9d3a6b..17be1b66cc41 100644
> >--- a/net/core/flow_dissector.c
> >+++ b/net/core/flow_dissector.c
> >@@ -246,13 +246,10 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> > 	case htons(ETH_P_8021AD):
> > 	case htons(ETH_P_8021Q): {
> > 		const struct vlan_hdr *vlan;
> >+		struct vlan_hdr _vlan;
> >+		bool vlan_tag_present = (skb && skb_vlan_tag_present(skb));
> 
> Drop the unnecessary "()"

ok

> > 
> >-		if (skb && skb_vlan_tag_present(skb))
> >-			proto = skb->protocol;
> 
> This does not look correct. I believe that you need to set proto for
> further processing.
> 

Ah, of course. I only looked at the usage in this 'case' statement,
but the variable is also used after the 'goto again' and at
the end of the function.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1506370

On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <jiri@resnulli.us> 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

[toc] | [prev] | [next] | [standalone]

#1506390

Sat, Oct 22, 2016 at 03:48:48AM CEST, torvalds@linux-foundation.org wrote:
>On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <jiri@resnulli.us> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web