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 <[email protected]>
---
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
Fri, Oct 21, 2016 at 05:55:53PM CEST, [email protected] 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 <[email protected]>
>---
>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
>
On Friday, October 21, 2016 6:31:18 PM CEST Jiri Pirko wrote:
> Fri, Oct 21, 2016 at 05:55:53PM CEST, [email protected] 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
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
On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <[email protected]> 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
Sat, Oct 22, 2016 at 03:48:48AM CEST, [email protected] wrote:
>On Fri, Oct 21, 2016 at 9:31 AM, Jiri Pirko <[email protected]> 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.
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.
On Sat, Oct 22, 2016 at 8:57 AM, Eric Garver <[email protected]> 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.
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 <[email protected]>
---
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);
Sat, Oct 22, 2016 at 10:30:08PM CEST, [email protected] 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 <[email protected]>
>---
>
>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);
>
On Monday, October 24, 2016 10:17:36 AM CEST Jiri Pirko wrote:
> Sat, Oct 22, 2016 at 10:30:08PM CEST, [email protected] 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