Re: [PATCH] flow_dissector: avoid uninitialized variable access
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
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
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
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
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
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
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 >
[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 --- 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