diff mbox series

[1/1] net:openvswitch: check return value of pskb_trim()

Message ID 20230717145024.27274-1-ruc_gongyuanjun@163.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [1/1] net:openvswitch: check return value of pskb_trim() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1342 this patch: 1343
netdev/cc_maintainers warning 3 maintainers not CCed: kuba@kernel.org dev@openvswitch.org pabeni@redhat.com
netdev/build_clang fail Errors and warnings before: 1365 this patch: 1367
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1365 this patch: 1366
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 13 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yuanjun Gong July 17, 2023, 2:50 p.m. UTC 
do kfree_skb() if an unexpected result is returned by pskb_tirm()
in do_output().

Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
---
 net/openvswitch/actions.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Eric Dumazet July 17, 2023, 3:53 p.m. UTC | #1 
On Mon, Jul 17, 2023 at 4:50 PM Yuanjun Gong <ruc_gongyuanjun@163.com> wrote:
>
> do kfree_skb() if an unexpected result is returned by pskb_tirm()
> in do_output().
>
> Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
> ---
>  net/openvswitch/actions.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index cab1e02b63e0..6b3456bdff1c 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -920,9 +920,11 @@ static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
>                 if (unlikely(cutlen > 0)) {
>                         if (skb->len - cutlen > ovs_mac_header_len(key))
> -                               pskb_trim(skb, skb->len - cutlen);
> +                               if (pskb_trim(skb, skb->len - cutlen))
> +                                       kfree_skb(skb);
>                         else
> -                               pskb_trim(skb, ovs_mac_header_len(key));
> +                               if (pskb_trim(skb, ovs_mac_header_len(key)))
> +                                       kfree_skb(skb);
>

This patch is not correct, skb will be reused later, and UAF will happen.
kernel test robot July 18, 2023, 3:28 p.m. UTC | #2 
Hi Yuanjun,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5-rc2 next-20230718]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yuanjun-Gong/net-openvswitch-check-return-value-of-pskb_trim/20230718-190417
base:   linus/master
patch link:    https://lore.kernel.org/r/20230717145024.27274-1-ruc_gongyuanjun%40163.com
patch subject: [PATCH 1/1] net:openvswitch: check return value of pskb_trim()
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230718/202307182349.2ivzwQk9-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230718/202307182349.2ivzwQk9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307182349.2ivzwQk9-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/openvswitch/actions.c: In function 'do_output':
>> net/openvswitch/actions.c:922:28: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
     922 |                         if (skb->len - cutlen > ovs_mac_header_len(key))
         |                            ^


vim +/else +922 net/openvswitch/actions.c

7f8a436eaa2c3d Joe Stringer      2015-08-26  911  
7f8a436eaa2c3d Joe Stringer      2015-08-26  912  static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
7f8a436eaa2c3d Joe Stringer      2015-08-26  913  		      struct sw_flow_key *key)
ccb1352e76cff0 Jesse Gross       2011-10-25  914  {
738967b8bf57e5 Andy Zhou         2014-09-08  915  	struct vport *vport = ovs_vport_rcu(dp, out_port);
ccb1352e76cff0 Jesse Gross       2011-10-25  916  
066b86787fa3d9 Felix Huettner    2023-04-05  917  	if (likely(vport && netif_carrier_ok(vport->dev))) {
7f8a436eaa2c3d Joe Stringer      2015-08-26  918  		u16 mru = OVS_CB(skb)->mru;
f2a4d086ed4c58 William Tu        2016-06-10  919  		u32 cutlen = OVS_CB(skb)->cutlen;
f2a4d086ed4c58 William Tu        2016-06-10  920  
f2a4d086ed4c58 William Tu        2016-06-10  921  		if (unlikely(cutlen > 0)) {
e2d9d8358cb961 Jiri Benc         2016-11-10 @922  			if (skb->len - cutlen > ovs_mac_header_len(key))
ec8358d8ed17bf Yuanjun Gong      2023-07-17  923  				if (pskb_trim(skb, skb->len - cutlen))
ec8358d8ed17bf Yuanjun Gong      2023-07-17  924  					kfree_skb(skb);
f2a4d086ed4c58 William Tu        2016-06-10  925  			else
ec8358d8ed17bf Yuanjun Gong      2023-07-17  926  				if (pskb_trim(skb, ovs_mac_header_len(key)))
ec8358d8ed17bf Yuanjun Gong      2023-07-17  927  					kfree_skb(skb);
f2a4d086ed4c58 William Tu        2016-06-10  928  		}
7f8a436eaa2c3d Joe Stringer      2015-08-26  929  
738314a084aae5 Jiri Benc         2016-11-10  930  		if (likely(!mru ||
738314a084aae5 Jiri Benc         2016-11-10  931  		           (skb->len <= mru + vport->dev->hard_header_len))) {
e2d9d8358cb961 Jiri Benc         2016-11-10  932  			ovs_vport_send(vport, skb, ovs_key_mac_proto(key));
7f8a436eaa2c3d Joe Stringer      2015-08-26  933  		} else if (mru <= vport->dev->mtu) {
c559cd3ad32ba7 Eric W. Biederman 2015-09-14  934  			struct net *net = read_pnet(&dp->net);
7f8a436eaa2c3d Joe Stringer      2015-08-26  935  
e2d9d8358cb961 Jiri Benc         2016-11-10  936  			ovs_fragment(net, vport, skb, mru, key);
7f8a436eaa2c3d Joe Stringer      2015-08-26  937  		} else {
7f8a436eaa2c3d Joe Stringer      2015-08-26  938  			kfree_skb(skb);
7f8a436eaa2c3d Joe Stringer      2015-08-26  939  		}
7f8a436eaa2c3d Joe Stringer      2015-08-26  940  	} else {
738967b8bf57e5 Andy Zhou         2014-09-08  941  		kfree_skb(skb);
ccb1352e76cff0 Jesse Gross       2011-10-25  942  	}
7f8a436eaa2c3d Joe Stringer      2015-08-26  943  }
ccb1352e76cff0 Jesse Gross       2011-10-25  944
kernel test robot July 19, 2023, 12:48 a.m. UTC | #3 
Hi Yuanjun,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5-rc2 next-20230718]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yuanjun-Gong/net-openvswitch-check-return-value-of-pskb_trim/20230718-190417
base:   linus/master
patch link:    https://lore.kernel.org/r/20230717145024.27274-1-ruc_gongyuanjun%40163.com
patch subject: [PATCH 1/1] net:openvswitch: check return value of pskb_trim()
config: loongarch-randconfig-r031-20230718 (https://download.01.org/0day-ci/archive/20230719/202307190802.c2sJufoJ-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230719/202307190802.c2sJufoJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307190802.c2sJufoJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/irqflags.h:18,
                    from include/linux/spinlock.h:59,
                    from include/linux/wait.h:9,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from include/linux/highmem.h:5,
                    from include/linux/bvec.h:10,
                    from include/linux/skbuff.h:17,
                    from net/openvswitch/actions.c:8:
   arch/loongarch/include/asm/percpu.h:20:4: error: #error compiler support for the model attribute is necessary when a recent assembler is used
      20 | #  error compiler support for the model attribute is necessary when a recent assembler is used
         |    ^~~~~
   In file included from include/linux/export.h:5,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:17,
                    from include/linux/skbuff.h:13:
   net/openvswitch/actions.c: In function 'do_output':
>> include/linux/compiler.h:55:26: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                          ^
   net/openvswitch/actions.c:922:25: note: in expansion of macro 'if'
     922 |                         if (skb->len - cutlen > ovs_mac_header_len(key))
         |                         ^~


vim +/else +55 include/linux/compiler.h

2bcd521a684cc9 Steven Rostedt 2008-11-21  49  
2bcd521a684cc9 Steven Rostedt 2008-11-21  50  #ifdef CONFIG_PROFILE_ALL_BRANCHES
2bcd521a684cc9 Steven Rostedt 2008-11-21  51  /*
2bcd521a684cc9 Steven Rostedt 2008-11-21  52   * "Define 'is'", Bill Clinton
2bcd521a684cc9 Steven Rostedt 2008-11-21  53   * "Define 'if'", Steven Rostedt
2bcd521a684cc9 Steven Rostedt 2008-11-21  54   */
a15fd609ad53a6 Linus Torvalds 2019-03-20 @55  #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
a15fd609ad53a6 Linus Torvalds 2019-03-20  56
diff mbox series

Patch

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index cab1e02b63e0..6b3456bdff1c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -920,9 +920,11 @@  static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
 
 		if (unlikely(cutlen > 0)) {
 			if (skb->len - cutlen > ovs_mac_header_len(key))
-				pskb_trim(skb, skb->len - cutlen);
+				if (pskb_trim(skb, skb->len - cutlen))
+					kfree_skb(skb);
 			else
-				pskb_trim(skb, ovs_mac_header_len(key));
+				if (pskb_trim(skb, ovs_mac_header_len(key)))
+					kfree_skb(skb);
 		}
 
 		if (likely(!mru ||