| Message ID | 20200618014328.28668-1-gaurav1086@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net/sched] Fix null pointer deref skb in tc_ctl_action | expand |
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 <gaurav1086@gmail.com> > --- > 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.
From: Gaurav Singh <gaurav1086@gmail.com> Date: Wed, 17 Jun 2020 21:43:28 -0400 > Add null check for skb > > Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> > --- > 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.
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 <lkp@intel.com>
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 1474 struct netlink_ext_ack *extack)
^1da177e4c3f41 Linus Torvalds 2005-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 Torvalds 2005-04-16 1482 int ret = 0, ovr = 0;
^1da177e4c3f41 Linus Torvalds 2005-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 Torvalds 2005-04-16 1495 return -EINVAL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1496 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1497
cc7ec456f82da7 Eric Dumazet 2011-01-19 1498 /* n->nlmsg_flags & NLM_F_CREATE */
^1da177e4c3f41 Linus Torvalds 2005-04-16 1499 switch (n->nlmsg_type) {
^1da177e4c3f41 Linus Torvalds 2005-04-16 1500 case RTM_NEWACTION:
^1da177e4c3f41 Linus Torvalds 2005-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 Torvalds 2005-04-16 1503 * Note that CREATE | EXCL implies that
^1da177e4c3f41 Linus Torvalds 2005-04-16 1504 * but since we want avoid ambiguity (eg when flags
^1da177e4c3f41 Linus Torvalds 2005-04-16 1505 * is zero) then just set this
^1da177e4c3f41 Linus Torvalds 2005-04-16 1506 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 1507 if (n->nlmsg_flags & NLM_F_REPLACE)
^1da177e4c3f41 Linus Torvalds 2005-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 Torvalds 2005-04-16 1511 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1512 case RTM_DELACTION:
7316ae88c43d47 Tom Goff 2010-03-19 1513 ret = tca_action_gd(net, tca[TCA_ACT_TAB], n,
84ae017a007764 Alexander Aring 2018-02-15 1514 portid, RTM_DELACTION, extack);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1515 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1516 case RTM_GETACTION:
7316ae88c43d47 Tom Goff 2010-03-19 1517 ret = tca_action_gd(net, tca[TCA_ACT_TAB], n,
84ae017a007764 Alexander Aring 2018-02-15 1518 portid, RTM_GETACTION, extack);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1519 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1520 default:
^1da177e4c3f41 Linus Torvalds 2005-04-16 1521 BUG();
^1da177e4c3f41 Linus Torvalds 2005-04-16 1522 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1523
^1da177e4c3f41 Linus Torvalds 2005-04-16 1524 return ret;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1525 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1526
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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) &&
Add null check for skb Signed-off-by: Gaurav Singh <gaurav1086@gmail.com> --- net/sched/act_api.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)