Re: [PATCH] [net/sched] Fix null pointer deref skb in tc_ctl_action

2020-06-19 Thread kernel test robot 
Hi Gaurav,

Thank you for the patch! Perhaps something to improve:

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

url:
https://github.com/0day-ci/linux/commits/Gaurav-Singh/Fix-null-pointer-deref-skb-in-tc_ctl_action/20200618-094734
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
1b5044021070efa3259f3e9548dc35d1eb6aa844
config: mips-randconfig-r004-20200619 (attached as .config)
compiler: mipsel-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=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>, old ones prefixed by <<):

net/sched/act_api.c: In function 'tc_ctl_action':
>> net/sched/act_api.c:1479:2: warning: ISO C90 forbids mixed declarations and 
>> code [-Wdeclaration-after-statement]
1479 |  struct net *net = sock_net(skb->sk);
|  ^~

vim +1479 net/sched/act_api.c

90825b23a887f0 Jamal Hadi Salim  2017-07-30  1472  
c21ef3e343ae91 David Ahern   2017-04-16  1473  static int 
tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
c21ef3e343ae91 David Ahern   2017-04-16  1474struct 
netlink_ext_ack *extack)
^1da177e4c3f41 Linus Torvalds2005-04-16  1475  {
63de5fbd31b331 Gaurav Singh  2020-06-17  1476   if (!skb)
63de5fbd31b331 Gaurav Singh  2020-06-17  1477   return 0;
63de5fbd31b331 Gaurav Singh  2020-06-17  1478  
3b1e0a655f8eba YOSHIFUJI Hideaki 2008-03-26 @1479   struct net *net = 
sock_net(skb->sk);
90825b23a887f0 Jamal Hadi Salim  2017-07-30  1480   struct nlattr 
*tca[TCA_ROOT_MAX + 1];
63de5fbd31b331 Gaurav Singh  2020-06-17  1481   u32 portid = 
NETLINK_CB(skb).portid;
^1da177e4c3f41 Linus Torvalds2005-04-16  1482   int ret = 0, ovr = 0;
^1da177e4c3f41 Linus Torvalds2005-04-16  1483  
0b0f43fe2e7291 Jamal Hadi Salim  2016-06-05  1484   if ((n->nlmsg_type != 
RTM_GETACTION) &&
0b0f43fe2e7291 Jamal Hadi Salim  2016-06-05  1485   
!netlink_capable(skb, CAP_NET_ADMIN))
dfc47ef8639fac Eric W. Biederman 2012-11-16  1486   return -EPERM;
dfc47ef8639fac Eric W. Biederman 2012-11-16  1487  
8cb081746c031f Johannes Berg 2019-04-26  1488   ret = 
nlmsg_parse_deprecated(n, sizeof(struct tcamsg), tca,
8cb081746c031f Johannes Berg 2019-04-26  1489   
 TCA_ROOT_MAX, NULL, extack);
7ba699c604ab81 Patrick McHardy   2008-01-22  1490   if (ret < 0)
7ba699c604ab81 Patrick McHardy   2008-01-22  1491   return ret;
7ba699c604ab81 Patrick McHardy   2008-01-22  1492  
7ba699c604ab81 Patrick McHardy   2008-01-22  1493   if (tca[TCA_ACT_TAB] == 
NULL) {
84ae017a007764 Alexander Aring   2018-02-15  1494   
NL_SET_ERR_MSG(extack, "Netlink action attributes missing");
^1da177e4c3f41 Linus Torvalds2005-04-16  1495   return -EINVAL;
^1da177e4c3f41 Linus Torvalds2005-04-16  1496   }
^1da177e4c3f41 Linus Torvalds2005-04-16  1497  
cc7ec456f82da7 Eric Dumazet  2011-01-19  1498   /* n->nlmsg_flags & 
NLM_F_CREATE */
^1da177e4c3f41 Linus Torvalds2005-04-16  1499   switch (n->nlmsg_type) {
^1da177e4c3f41 Linus Torvalds2005-04-16  1500   case RTM_NEWACTION:
^1da177e4c3f41 Linus Torvalds2005-04-16  1501   /* we are going 
to assume all other flags
25985edcedea63 Lucas De Marchi   2011-03-30  1502* imply create 
only if it doesn't exist
^1da177e4c3f41 Linus Torvalds2005-04-16  1503* Note that 
CREATE | EXCL implies that
^1da177e4c3f41 Linus Torvalds2005-04-16  1504* but since we 
want avoid ambiguity (eg when flags
^1da177e4c3f41 Linus Torvalds2005-04-16  1505* is zero) 
then just set this
^1da177e4c3f41 Linus Torvalds2005-04-16  1506*/
^1da177e4c3f41 Linus Torvalds2005-04-16  1507   if 
(n->nlmsg_flags & NLM_F_REPLACE)
^1da177e4c3f41 Linus Torvalds2005-04-16  1508   ovr = 1;
aea0d727899140 Alexander Aring   2018-02-15  1509   ret = 
tcf_action_add(net, tca[TCA_ACT_TAB], n, portid, ovr,
aea0d727899140 Alexander Aring   2018-02-15  1510   
 extack);
^1da177e4c3f41 Linus Torvalds2005-04-16  1511   break;
^1da177e4c3f41 Linus Torvalds2005-04-16  1512   case RTM_DELACTION:
7316ae88c43d47 Tom Goff  2010-03-19  1513   ret = 
tca_action_gd(net, tca

Re: [PATCH] [net/sched] Fix null pointer deref skb in tc_ctl_action

2020-06-18 Thread David Miller 
From: Gaurav Singh 
Date: Wed, 17 Jun 2020 21:43:28 -0400

> Add null check for skb
> 
> Signed-off-by: Gaurav Singh 
> ---
>  net/sched/act_api.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8ac7eb0a8309..fd584821d75a 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1473,9 +1473,12 @@ static const struct nla_policy 
> tcaa_policy[TCA_ROOT_MAX + 1] = {
>  static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>struct netlink_ext_ack *extack)
>  {
> + if (!skb)
> + return 0;
> +
>   struct net *net = sock_net(skb->sk);

You're adding code before variable declarations, this is not correct.

I find your work to be very rushed and sloppy, please take your time
and submit well formed and tested changes.

Thank you.

Re: [PATCH] [net/sched] Fix null pointer deref skb in tc_ctl_action

2020-06-17 Thread Eric Dumazet 



On 6/17/20 6:43 PM, Gaurav Singh wrote:
> Add null check for skb
> 

Bad choice really.

You have to really understand code intent before trying to fix it.

> Signed-off-by: Gaurav Singh 
> ---
>  net/sched/act_api.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8ac7eb0a8309..fd584821d75a 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1473,9 +1473,12 @@ static const struct nla_policy 
> tcaa_policy[TCA_ROOT_MAX + 1] = {
>  static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>struct netlink_ext_ack *extack)
>  {
> + if (!skb)
> + return 0;


We do not allow this

warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]

> +
>   struct net *net = sock_net(skb->sk);
>   struct nlattr *tca[TCA_ROOT_MAX + 1];
> - u32 portid = skb ? NETLINK_CB(skb).portid : 0;
> + u32 portid = NETLINK_CB(skb).portid;
>   int ret = 0, ovr = 0;
>  
>   if ((n->nlmsg_type != RTM_GETACTION) &&
> 

Please compile your patches, do not expect us from doing this.

[PATCH] [net/sched] Fix null pointer deref skb in tc_ctl_action

2020-06-17 Thread Gaurav Singh 
Add null check for skb

Signed-off-by: Gaurav Singh 
---
 net/sched/act_api.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8ac7eb0a8309..fd584821d75a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1473,9 +1473,12 @@ static const struct nla_policy tcaa_policy[TCA_ROOT_MAX 
+ 1] = {
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 struct netlink_ext_ack *extack)
 {
+   if (!skb)
+   return 0;
+
struct net *net = sock_net(skb->sk);
struct nlattr *tca[TCA_ROOT_MAX + 1];
-   u32 portid = skb ? NETLINK_CB(skb).portid : 0;
+   u32 portid = NETLINK_CB(skb).portid;
int ret = 0, ovr = 0;
 
if ((n->nlmsg_type != RTM_GETACTION) &&
-- 
2.17.1