[PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Gaurav Singh]

ipv6_pinfo is initlialized by inet6_sk() which returns NULL. 
Hence it can cause segmentation fault. Fix this by adding a 
NULL check.

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/ipv6/ip6_output.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..7c077a6847e4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
 {
-	if (!np->autoflowlabel_set)
-		return ip6_default_np_autolabel(net);
-	else
+	if (np && np->autoflowlabel_set)
 		return np->autoflowlabel;
+	else
+		ip6_default_np_autolabel(net);
 }
 
 /*
-- 
2.17.1

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Hi Gaurav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sparc-next/master]
[also build test WARNING on ipvs/master linus/master v5.8-rc7 next-20200724]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Gaurav-Singh/ip6_output-Add-ipv6_pinfo-null-check/20200727-113949
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-next.git master
config: csky-defconfig (attached as .config)
compiler: csky-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=csky 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/ipv6/ip6_output.c: In function 'ip6_autoflowlabel':
>> net/ipv6/ip6_output.c:188:1: warning: control reaches end of non-void function [-Wreturn-type]
     188 | }
         | ^

vim +188 net/ipv6/ip6_output.c

^1da177e4c3f41 Linus Torvalds 2005-04-16  181  
e9191ffb65d8e1 Ben Hutchings  2018-01-22  182  bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
513674b5a2c9c7 Shaohua Li     2017-12-20  183  {
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  184  	if (np && np->autoflowlabel_set)
513674b5a2c9c7 Shaohua Li     2017-12-20  185  		return np->autoflowlabel;
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  186  	else
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  187  		ip6_default_np_autolabel(net);
513674b5a2c9c7 Shaohua Li     2017-12-20 @188  }
513674b5a2c9c7 Shaohua Li     2017-12-20  189  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10045 bytes --]

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

Hi Gaurav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sparc-next/master]
[also build test WARNING on ipvs/master linus/master v5.8-rc7 next-20200724]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Gaurav-Singh/ip6_output-Add-ipv6_pinfo-null-check/20200727-113949
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-next.git master
config: x86_64-randconfig-r011-20200727 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 8dc820393219c7ee440b4ec86c9a201301943276)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ipv6/ip6_output.c:188:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
   }
   ^
   1 warning generated.

vim +188 net/ipv6/ip6_output.c

^1da177e4c3f41 Linus Torvalds 2005-04-16  181  
e9191ffb65d8e1 Ben Hutchings  2018-01-22  182  bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
513674b5a2c9c7 Shaohua Li     2017-12-20  183  {
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  184  	if (np && np->autoflowlabel_set)
513674b5a2c9c7 Shaohua Li     2017-12-20  185  		return np->autoflowlabel;
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  186  	else
5bdc1ea8a7d229 Gaurav Singh   2020-07-26  187  		ip6_default_np_autolabel(net);
513674b5a2c9c7 Shaohua Li     2017-12-20 @188  }
513674b5a2c9c7 Shaohua Li     2017-12-20  189  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29465 bytes --]

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[David Miller]

From: Gaurav Singh <gaurav1086@gmail.com>
Date: Sun, 26 Jul 2020 23:38:10 -0400

> ipv6_pinfo is initlialized by inet6_sk() which returns NULL. 
> Hence it can cause segmentation fault. Fix this by adding a 
> NULL check.

Please take your time with such changes and actually look at the
compiler output, it will warn that you are adding a new problem.

Specifically, the function now fails to return a value at all.

But even more important, how is the situation you are fixing
possible?  Do you have a sample crash?  Can you please describe
the code paths and conditions that lead to the problem?

You must also provide a valid and properly formatted Fixes: tag
for bug fixes such as this.

Thank you.

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Cong Wang]

On Sun, Jul 26, 2020 at 8:39 PM Gaurav Singh <gaurav1086@gmail.com> wrote:
>
> ipv6_pinfo is initlialized by inet6_sk() which returns NULL.

Why? It only returns NULL for timewait or request sock, but
I don't see how ip6_autoflowlabel() could be called on these
sockets. So please explain.

> Hence it can cause segmentation fault. Fix this by adding a
> NULL check.

Which exact call path? Do you have a full stack trace?

Thanks.

[PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Gaurav Singh]

Add return to fix build issue. Haven't reproduced this issue at
my end. 

My hypothesis is this: In function: ip6_xmit(), we have
const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.

Further down the function, there's a check:
if (np) hlimit = hp->htop_limit 

Further, we have a call
ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
ip6_autoflowlabel(net, np), fl6)); . 

Hence np = NULL gets passed in
the function ip6_autoflowlabel() which accesses np-> without check which
may cause a segment violation.

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/ipv6/ip6_output.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..94a07c9bd925 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
 {
-	if (!np->autoflowlabel_set)
-		return ip6_default_np_autolabel(net);
-	else
+	if (np && np->autoflowlabel_set)
 		return np->autoflowlabel;
+	else
+		return ip6_default_np_autolabel(net);
 }
 
 /*
-- 
2.17.1

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Cong Wang]

On Mon, Jul 27, 2020 at 7:14 PM Gaurav Singh <gaurav1086@gmail.com> wrote:
>
> Add return to fix build issue. Haven't reproduced this issue at
> my end.
>
> My hypothesis is this: In function: ip6_xmit(), we have
> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.
>
> Further down the function, there's a check:
> if (np) hlimit = hp->htop_limit

This check exists before git history, at that time 'sk' could be NULL,
hence 'np', so it does not mean it is still necessary now.

I looked at all callers of ip6_xmit(), I don't see how it is called with
a non-full socket, neither 'sk' could be NULL after
commit b30bd282cbf5c46247a279a2e8d2aae027d9f1bf
("[IPV6]: ip6_xmit: remove unnecessary NULL ptr check").

Thanks.

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Eric Dumazet]

On 7/27/20 8:12 PM, Cong Wang wrote:
> On Mon, Jul 27, 2020 at 7:14 PM Gaurav Singh <gaurav1086@gmail.com> wrote:
>>
>> Add return to fix build issue. Haven't reproduced this issue at
>> my end.
>>
>> My hypothesis is this: In function: ip6_xmit(), we have
>> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.
>>
>> Further down the function, there's a check:
>> if (np) hlimit = hp->htop_limit
> 
> This check exists before git history, at that time 'sk' could be NULL,
> hence 'np', so it does not mean it is still necessary now.
> 
> I looked at all callers of ip6_xmit(), I don't see how it is called with
> a non-full socket, neither 'sk' could be NULL after
> commit b30bd282cbf5c46247a279a2e8d2aae027d9f1bf
> ("[IPV6]: ip6_xmit: remove unnecessary NULL ptr check").
> 
> Thanks.
> 


Agreed.

And again, fact that this patch lacks a Fixes:  tag speaks for itself.

This means the author expects all reviewers to make a deep analysis.

Please bear with us, and add a Fixes: tag so that we can fully understand what was
the bug origin and why a fix is valid.

[PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Gaurav Singh]

This PR fixes a possible segmentation violation.

In function: ip6_xmit(), we have
const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL
unconditionally (regardless sk being  NULL or not).

In include/linux/ipv6.h:

static inline struct ipv6_pinfo * inet6_sk(const struct sock *__sk)
{
    return NULL;
}

Further down the function, there's a check:
if (np) hlimit = hp->htop_limit

Thereafter, we have a call
ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
ip6_autoflowlabel(net, np), fl6));

Hence np = NULL gets passed in
the function ip6_autoflowlabel() that accesses np without check which
may cause a segment violation.

Fixes: 513674b5a2c9c ("net: reevalulate autoflowlabel setting after sysctl setting")

Signed-off-by: Gaurav Singh <gaurav1086@gmail.com>
---
 net/ipv6/ip6_output.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..94a07c9bd925 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
 {
-	if (!np->autoflowlabel_set)
-		return ip6_default_np_autolabel(net);
-	else
+	if (np && np->autoflowlabel_set)
 		return np->autoflowlabel;
+	else
+		return ip6_default_np_autolabel(net);
 }
 
 /*
-- 
2.17.1

Re: [PATCH] [net/ipv6] ip6_output: Add ipv6_pinfo null check

[Cong Wang]

On Sat, Aug 8, 2020 at 10:07 AM Gaurav Singh <gaurav1086@gmail.com> wrote:
>
> This PR fixes a possible segmentation violation.
>
> In function: ip6_xmit(), we have
> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL
> unconditionally (regardless sk being  NULL or not).
>
> In include/linux/ipv6.h:
>
> static inline struct ipv6_pinfo * inet6_sk(const struct sock *__sk)
> {
>     return NULL;
> }
>

Tell us who will use ip6_autoflowlabel() when CONFIG_IPV6
is disabled. :)