diff mbox series

ax25: Fix use of copy_from_sockptr() in ax25_setsockopt()

Message ID YVXkwzKZhPoD0Ods@linux-mips.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ax25: Fix use of copy_from_sockptr() in ax25_setsockopt() | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers fail 2 blamed authors not CCed: stefan@datenfreihafen.org matthieu.baerts@tessares.net; 3 maintainers not CCed: stefan@datenfreihafen.org matthieu.baerts@tessares.net jreuter@yaina.de
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Ralf Baechle Sept. 30, 2021, 4:24 p.m. UTC
The destination pointer passed to copy_from_sockptr() is an unsigned long *
but the source in userspace is an unsigned int * resulting in an integer
of the wrong size being copied from userspace.

This happens to work on 32 bit but breaks 64-bit where bytes 4..7 will not
be initialized.  By luck it may work on little endian but on big endian
where the userspace data is copied to the upper 32 bit of the destination
it's most likely going to break.

A simple test case to demonstrate this setsockopt() issue is:

[...]
        int sk = socket(AF_AX25, SOCK_SEQPACKET, 0);
        int n1 = 42;
        int res = setsockopt(sk, SOL_AX25, AX25_T1, &n1, sizeof(n1));
        printf("res = %d\n", res);
[...]

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Cc: stable@vger.kernel.org # 5.9
Fixes: a7b75c5a8c41 ("net: pass a sockptr_t into ->setsockopt")
---
 net/ax25/af_ax25.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

kernel test robot Oct. 1, 2021, 2:31 a.m. UTC | #1
Hi Ralf,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master horms-ipvs/master linus/master v5.15-rc3 next-20210922]
[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/Ralf-Baechle/ax25-Fix-use-of-copy_from_sockptr-in-ax25_setsockopt/20211001-002911
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 35306eb23814444bd4021f8a1c3047d3cb0c8b2b
config: x86_64-randconfig-r012-20210930 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4)
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
        # https://github.com/0day-ci/linux/commit/a84e2204c4c268dd4475a95ed472285982ae6c57
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ralf-Baechle/ax25-Fix-use-of-copy_from_sockptr-in-ax25_setsockopt/20211001-002911
        git checkout a84e2204c4c268dd4475a95ed472285982ae6c57
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 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/ax25/af_ax25.c:569:22: warning: result of comparison of constant 18446744073709551 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (opt < 1 || opt > ULONG_MAX / HZ) {
                                  ~~~ ^ ~~~~~~~~~~~~~~
   net/ax25/af_ax25.c:578:22: warning: result of comparison of constant 18446744073709551 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (opt < 1 || opt > ULONG_MAX / HZ) {
                                  ~~~ ^ ~~~~~~~~~~~~~~
   net/ax25/af_ax25.c:594:22: warning: result of comparison of constant 18446744073709551 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (opt < 1 || opt > ULONG_MAX / HZ) {
                                  ~~~ ^ ~~~~~~~~~~~~~~
   net/ax25/af_ax25.c:602:11: warning: result of comparison of constant 307445734561825 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
                   if (opt > ULONG_MAX / (60 * HZ)) {
                       ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +569 net/ax25/af_ax25.c

^1da177e4c3f41 Linus Torvalds    2005-04-16  524  
^1da177e4c3f41 Linus Torvalds    2005-04-16  525  /*
^1da177e4c3f41 Linus Torvalds    2005-04-16  526   *	Handling for system calls applied via the various interfaces to an
^1da177e4c3f41 Linus Torvalds    2005-04-16  527   *	AX25 socket object
^1da177e4c3f41 Linus Torvalds    2005-04-16  528   */
^1da177e4c3f41 Linus Torvalds    2005-04-16  529  
^1da177e4c3f41 Linus Torvalds    2005-04-16  530  static int ax25_setsockopt(struct socket *sock, int level, int optname,
a7b75c5a8c4144 Christoph Hellwig 2020-07-23  531  		sockptr_t optval, unsigned int optlen)
^1da177e4c3f41 Linus Torvalds    2005-04-16  532  {
^1da177e4c3f41 Linus Torvalds    2005-04-16  533  	struct sock *sk = sock->sk;
^1da177e4c3f41 Linus Torvalds    2005-04-16  534  	ax25_cb *ax25;
^1da177e4c3f41 Linus Torvalds    2005-04-16  535  	struct net_device *dev;
^1da177e4c3f41 Linus Torvalds    2005-04-16  536  	char devname[IFNAMSIZ];
a84e2204c4c268 Ralf Baechle      2021-09-30  537  	unsigned int opt;
ba1cffe0257bcd Xi Wang           2011-12-27  538  	int res = 0;
^1da177e4c3f41 Linus Torvalds    2005-04-16  539  
^1da177e4c3f41 Linus Torvalds    2005-04-16  540  	if (level != SOL_AX25)
^1da177e4c3f41 Linus Torvalds    2005-04-16  541  		return -ENOPROTOOPT;
^1da177e4c3f41 Linus Torvalds    2005-04-16  542  
ba1cffe0257bcd Xi Wang           2011-12-27  543  	if (optlen < sizeof(unsigned int))
^1da177e4c3f41 Linus Torvalds    2005-04-16  544  		return -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  545  
a7b75c5a8c4144 Christoph Hellwig 2020-07-23  546  	if (copy_from_sockptr(&opt, optval, sizeof(unsigned int)))
^1da177e4c3f41 Linus Torvalds    2005-04-16  547  		return -EFAULT;
^1da177e4c3f41 Linus Torvalds    2005-04-16  548  
^1da177e4c3f41 Linus Torvalds    2005-04-16  549  	lock_sock(sk);
3200392b88dd25 David Miller      2015-06-25  550  	ax25 = sk_to_ax25(sk);
^1da177e4c3f41 Linus Torvalds    2005-04-16  551  
^1da177e4c3f41 Linus Torvalds    2005-04-16  552  	switch (optname) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  553  	case AX25_WINDOW:
^1da177e4c3f41 Linus Torvalds    2005-04-16  554  		if (ax25->modulus == AX25_MODULUS) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  555  			if (opt < 1 || opt > 7) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  556  				res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  557  				break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  558  			}
^1da177e4c3f41 Linus Torvalds    2005-04-16  559  		} else {
^1da177e4c3f41 Linus Torvalds    2005-04-16  560  			if (opt < 1 || opt > 63) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  561  				res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  562  				break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  563  			}
^1da177e4c3f41 Linus Torvalds    2005-04-16  564  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  565  		ax25->window = opt;
^1da177e4c3f41 Linus Torvalds    2005-04-16  566  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  567  
^1da177e4c3f41 Linus Torvalds    2005-04-16  568  	case AX25_T1:
be639ac6901a08 Ralf Baechle      2011-11-24 @569  		if (opt < 1 || opt > ULONG_MAX / HZ) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  570  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  571  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  572  		}
f16f3026db6fa6 Eric Dumazet      2008-01-13  573  		ax25->rtt = (opt * HZ) >> 1;
^1da177e4c3f41 Linus Torvalds    2005-04-16  574  		ax25->t1  = opt * HZ;
^1da177e4c3f41 Linus Torvalds    2005-04-16  575  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  576  
^1da177e4c3f41 Linus Torvalds    2005-04-16  577  	case AX25_T2:
be639ac6901a08 Ralf Baechle      2011-11-24  578  		if (opt < 1 || opt > ULONG_MAX / HZ) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  579  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  580  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  581  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  582  		ax25->t2 = opt * HZ;
^1da177e4c3f41 Linus Torvalds    2005-04-16  583  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  584  
^1da177e4c3f41 Linus Torvalds    2005-04-16  585  	case AX25_N2:
^1da177e4c3f41 Linus Torvalds    2005-04-16  586  		if (opt < 1 || opt > 31) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  587  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  588  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  589  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  590  		ax25->n2 = opt;
^1da177e4c3f41 Linus Torvalds    2005-04-16  591  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  592  
^1da177e4c3f41 Linus Torvalds    2005-04-16  593  	case AX25_T3:
be639ac6901a08 Ralf Baechle      2011-11-24  594  		if (opt < 1 || opt > ULONG_MAX / HZ) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  595  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  596  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  597  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  598  		ax25->t3 = opt * HZ;
^1da177e4c3f41 Linus Torvalds    2005-04-16  599  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  600  
^1da177e4c3f41 Linus Torvalds    2005-04-16  601  	case AX25_IDLE:
ba1cffe0257bcd Xi Wang           2011-12-27  602  		if (opt > ULONG_MAX / (60 * HZ)) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  603  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  604  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  605  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  606  		ax25->idle = opt * 60 * HZ;
^1da177e4c3f41 Linus Torvalds    2005-04-16  607  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  608  
^1da177e4c3f41 Linus Torvalds    2005-04-16  609  	case AX25_BACKOFF:
ba1cffe0257bcd Xi Wang           2011-12-27  610  		if (opt > 2) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  611  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  612  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  613  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  614  		ax25->backoff = opt;
^1da177e4c3f41 Linus Torvalds    2005-04-16  615  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  616  
^1da177e4c3f41 Linus Torvalds    2005-04-16  617  	case AX25_EXTSEQ:
^1da177e4c3f41 Linus Torvalds    2005-04-16  618  		ax25->modulus = opt ? AX25_EMODULUS : AX25_MODULUS;
^1da177e4c3f41 Linus Torvalds    2005-04-16  619  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  620  
^1da177e4c3f41 Linus Torvalds    2005-04-16  621  	case AX25_PIDINCL:
^1da177e4c3f41 Linus Torvalds    2005-04-16  622  		ax25->pidincl = opt ? 1 : 0;
^1da177e4c3f41 Linus Torvalds    2005-04-16  623  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  624  
^1da177e4c3f41 Linus Torvalds    2005-04-16  625  	case AX25_IAMDIGI:
^1da177e4c3f41 Linus Torvalds    2005-04-16  626  		ax25->iamdigi = opt ? 1 : 0;
^1da177e4c3f41 Linus Torvalds    2005-04-16  627  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  628  
^1da177e4c3f41 Linus Torvalds    2005-04-16  629  	case AX25_PACLEN:
^1da177e4c3f41 Linus Torvalds    2005-04-16  630  		if (opt < 16 || opt > 65535) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  631  			res = -EINVAL;
^1da177e4c3f41 Linus Torvalds    2005-04-16  632  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  633  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  634  		ax25->paclen = opt;
^1da177e4c3f41 Linus Torvalds    2005-04-16  635  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  636  
^1da177e4c3f41 Linus Torvalds    2005-04-16  637  	case SO_BINDTODEVICE:
687775cec056b3 Eric Dumazet      2020-05-19  638  		if (optlen > IFNAMSIZ - 1)
687775cec056b3 Eric Dumazet      2020-05-19  639  			optlen = IFNAMSIZ - 1;
687775cec056b3 Eric Dumazet      2020-05-19  640  
687775cec056b3 Eric Dumazet      2020-05-19  641  		memset(devname, 0, sizeof(devname));
2f72291d3d0e44 Ralf Baechle      2009-09-28  642  
a7b75c5a8c4144 Christoph Hellwig 2020-07-23  643  		if (copy_from_sockptr(devname, optval, optlen)) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  644  			res = -EFAULT;
^1da177e4c3f41 Linus Torvalds    2005-04-16  645  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  646  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  647  
^1da177e4c3f41 Linus Torvalds    2005-04-16  648  		if (sk->sk_type == SOCK_SEQPACKET &&
^1da177e4c3f41 Linus Torvalds    2005-04-16  649  		   (sock->state != SS_UNCONNECTED ||
^1da177e4c3f41 Linus Torvalds    2005-04-16  650  		    sk->sk_state == TCP_LISTEN)) {
^1da177e4c3f41 Linus Torvalds    2005-04-16  651  			res = -EADDRNOTAVAIL;
2f72291d3d0e44 Ralf Baechle      2009-09-28  652  			break;
2f72291d3d0e44 Ralf Baechle      2009-09-28  653  		}
2f72291d3d0e44 Ralf Baechle      2009-09-28  654  
c433570458e49b Cong Wang         2018-12-29  655  		rtnl_lock();
c433570458e49b Cong Wang         2018-12-29  656  		dev = __dev_get_by_name(&init_net, devname);
2f72291d3d0e44 Ralf Baechle      2009-09-28  657  		if (!dev) {
c433570458e49b Cong Wang         2018-12-29  658  			rtnl_unlock();
2f72291d3d0e44 Ralf Baechle      2009-09-28  659  			res = -ENODEV;
^1da177e4c3f41 Linus Torvalds    2005-04-16  660  			break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  661  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  662  
^1da177e4c3f41 Linus Torvalds    2005-04-16  663  		ax25->ax25_dev = ax25_dev_ax25dev(dev);
c433570458e49b Cong Wang         2018-12-29  664  		if (!ax25->ax25_dev) {
c433570458e49b Cong Wang         2018-12-29  665  			rtnl_unlock();
c433570458e49b Cong Wang         2018-12-29  666  			res = -ENODEV;
c433570458e49b Cong Wang         2018-12-29  667  			break;
c433570458e49b Cong Wang         2018-12-29  668  		}
^1da177e4c3f41 Linus Torvalds    2005-04-16  669  		ax25_fillin_cb(ax25, ax25->ax25_dev);
c433570458e49b Cong Wang         2018-12-29  670  		rtnl_unlock();
^1da177e4c3f41 Linus Torvalds    2005-04-16  671  		break;
^1da177e4c3f41 Linus Torvalds    2005-04-16  672  
^1da177e4c3f41 Linus Torvalds    2005-04-16  673  	default:
^1da177e4c3f41 Linus Torvalds    2005-04-16  674  		res = -ENOPROTOOPT;
^1da177e4c3f41 Linus Torvalds    2005-04-16  675  	}
^1da177e4c3f41 Linus Torvalds    2005-04-16  676  	release_sock(sk);
^1da177e4c3f41 Linus Torvalds    2005-04-16  677  
^1da177e4c3f41 Linus Torvalds    2005-04-16  678  	return res;
^1da177e4c3f41 Linus Torvalds    2005-04-16  679  }
^1da177e4c3f41 Linus Torvalds    2005-04-16  680  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
hch@lst.de Oct. 12, 2021, 6:23 a.m. UTC | #2
On Thu, Sep 30, 2021 at 06:24:35PM +0200, Ralf Baechle wrote:
> The destination pointer passed to copy_from_sockptr() is an unsigned long *
> but the source in userspace is an unsigned int * resulting in an integer
> of the wrong size being copied from userspace.
> 
> This happens to work on 32 bit but breaks 64-bit where bytes 4..7 will not
> be initialized.  By luck it may work on little endian but on big endian
> where the userspace data is copied to the upper 32 bit of the destination
> it's most likely going to break.
> 
> A simple test case to demonstrate this setsockopt() issue is:

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Ralf Baechle Oct. 12, 2021, 5:10 p.m. UTC | #3
On Tue, Oct 12, 2021 at 08:23:09AM +0200, Christoph Hellwig wrote:

> On Thu, Sep 30, 2021 at 06:24:35PM +0200, Ralf Baechle wrote:
> > The destination pointer passed to copy_from_sockptr() is an unsigned long *
> > but the source in userspace is an unsigned int * resulting in an integer
> > of the wrong size being copied from userspace.
> > 
> > This happens to work on 32 bit but breaks 64-bit where bytes 4..7 will not
> > be initialized.  By luck it may work on little endian but on big endian
> > where the userspace data is copied to the upper 32 bit of the destination
> > it's most likely going to break.
> > 
> > A simple test case to demonstrate this setsockopt() issue is:
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Sadly the kernel test robot has raised a bunch of warnings in this patch.
To fix those I'll pull a few fixes from another patch I was planning to
send later and merge them into this patch and post the resulting patch
as v2.

Thanks for the review,

  Ralf
diff mbox series

Patch

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 2631efc6e359..9f2e4b76394a 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -534,7 +534,7 @@  static int ax25_setsockopt(struct socket *sock, int level, int optname,
 	ax25_cb *ax25;
 	struct net_device *dev;
 	char devname[IFNAMSIZ];
-	unsigned long opt;
+	unsigned int opt;
 	int res = 0;
 
 	if (level != SOL_AX25)