diff mbox series

[v3,1/2] userfaultfd: do not untag user pointers

Message ID 20210702225705.2477947-2-pcc@google.com (mailing list archive)
State New
Headers show
Series userfaultfd: do not untag user pointers | expand

Commit Message

Peter Collingbourne July 2, 2021, 10:57 p.m. UTC 
If a user program uses userfaultfd on ranges of heap memory, it may
end up passing a tagged pointer to the kernel in the range.start
field of the UFFDIO_REGISTER ioctl. This can happen when using an
MTE-capable allocator, or on Android if using the Tagged Pointers
feature for MTE readiness [1].

When a fault subsequently occurs, the tag is stripped from the fault
address returned to the application in the fault.address field
of struct uffd_msg. However, from the application's perspective,
the tagged address *is* the memory address, so if the application
is unaware of memory tags, it may get confused by receiving an
address that is, from its point of view, outside of the bounds of the
allocation. We observed this behavior in the kselftest for userfaultfd
[2] but other applications could have the same problem.

Address this by not untagging pointers passed to the userfaultfd
ioctls. Instead, let the system call fail. This will provide an
early indication of problems with tag-unaware userspace code instead
of letting the code get confused later, and is consistent with how
we decided to handle brk/mmap/mremap in commit dcde237319e6 ("mm:
Avoid creating virtual address aliases in brk()/mmap()/mremap()"),
as well as being consistent with the existing tagged address ABI
documentation relating to how ioctl arguments are handled.

The code change is a revert of commit 7d0325749a6c ("userfaultfd:
untag user pointers").

[1] https://source.android.com/devices/tech/debug/tagged-pointers
[2] tools/testing/selftests/vm/userfaultfd.c

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/I761aa9f0344454c482b83fcfcce547db0a25501b
Fixes: 63f0c6037965 ("arm64: Introduce prctl() options to control the tagged user addresses ABI")
Cc: <stable@vger.kernel.org> # 5.4
---
 Documentation/arm64/tagged-address-abi.rst | 25 +++++++++++++++-------
 fs/userfaultfd.c                           | 22 +++++++++----------
 2 files changed, 27 insertions(+), 20 deletions(-)

Comments

kernel test robot July 3, 2021, 2:46 a.m. UTC | #1 
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on kselftest/next linus/master v5.13 next-20210701]
[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/Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: x86_64-randconfig-s032-20210702 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/42177546697fa573571799dc11ecd12a65449886
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
        git checkout 42177546697fa573571799dc11ecd12a65449886
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

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


sparse warnings: (new ones prefixed by >>)
>> fs/userfaultfd.c:1816:40: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned long long [usertype] start @@     got unsigned long long * @@
   fs/userfaultfd.c:1816:40: sparse:     expected unsigned long long [usertype] start
   fs/userfaultfd.c:1816:40: sparse:     got unsigned long long *
   fs/userfaultfd.c:1864:40: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned long long [usertype] start @@     got unsigned long long * @@
   fs/userfaultfd.c:1864:40: sparse:     expected unsigned long long [usertype] start
   fs/userfaultfd.c:1864:40: sparse:     got unsigned long long *

vim +1816 fs/userfaultfd.c

ad465cae96b456 Andrea Arcangeli 2015-09-04  1797  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1798  static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1799  				    unsigned long arg)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1800  {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1801  	int ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1802  	struct uffdio_writeprotect uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1803  	struct uffdio_writeprotect __user *user_uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1804  	struct userfaultfd_wake_range range;
23080e2783ba45 Peter Xu         2020-04-06  1805  	bool mode_wp, mode_dontwake;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1806  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1807  	if (READ_ONCE(ctx->mmap_changing))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1808  		return -EAGAIN;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1809  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1810  	user_uffdio_wp = (struct uffdio_writeprotect __user *) arg;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1811  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1812  	if (copy_from_user(&uffdio_wp, user_uffdio_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1813  			   sizeof(struct uffdio_writeprotect)))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1814  		return -EFAULT;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1815  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06 @1816  	ret = validate_range(ctx->mm, &uffdio_wp.range.start,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1817  			     uffdio_wp.range.len);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1818  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1819  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1820  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1821  	if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1822  			       UFFDIO_WRITEPROTECT_MODE_WP))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1823  		return -EINVAL;
23080e2783ba45 Peter Xu         2020-04-06  1824  
23080e2783ba45 Peter Xu         2020-04-06  1825  	mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
23080e2783ba45 Peter Xu         2020-04-06  1826  	mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
23080e2783ba45 Peter Xu         2020-04-06  1827  
23080e2783ba45 Peter Xu         2020-04-06  1828  	if (mode_wp && mode_dontwake)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1829  		return -EINVAL;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1830  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1831  	ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
23080e2783ba45 Peter Xu         2020-04-06  1832  				  uffdio_wp.range.len, mode_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1833  				  &ctx->mmap_changing);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1834  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1835  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1836  
23080e2783ba45 Peter Xu         2020-04-06  1837  	if (!mode_wp && !mode_dontwake) {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1838  		range.start = uffdio_wp.range.start;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1839  		range.len = uffdio_wp.range.len;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1840  		wake_userfault(ctx, &range);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1841  	}
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1842  	return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1843  }
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1844  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot July 3, 2021, 3:06 a.m. UTC | #2 
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on kselftest/next linus/master v5.13 next-20210701]
[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/Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: x86_64-randconfig-r034-20210702 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project cb5de7c813f976dd458bd2a7f40702ba648bf650)
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
        # https://github.com/0day-ci/linux/commit/42177546697fa573571799dc11ecd12a65449886
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
        git checkout 42177546697fa573571799dc11ecd12a65449886
        # 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 >>):

>> fs/userfaultfd.c:1816:32: warning: incompatible pointer to integer conversion passing '__u64 *' (aka 'unsigned long long *') to parameter of type '__u64' (aka 'unsigned long long'); remove & [-Wint-conversion]
           ret = validate_range(ctx->mm, &uffdio_wp.range.start,
                                         ^~~~~~~~~~~~~~~~~~~~~~
   fs/userfaultfd.c:1239:14: note: passing argument to parameter 'start' here
                                             __u64 start, __u64 len)
                                                   ^
   fs/userfaultfd.c:1864:32: warning: incompatible pointer to integer conversion passing '__u64 *' (aka 'unsigned long long *') to parameter of type '__u64' (aka 'unsigned long long'); remove & [-Wint-conversion]
           ret = validate_range(ctx->mm, &uffdio_continue.range.start,
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/userfaultfd.c:1239:14: note: passing argument to parameter 'start' here
                                             __u64 start, __u64 len)
                                                   ^
   2 warnings generated.


vim +1816 fs/userfaultfd.c

ad465cae96b456 Andrea Arcangeli 2015-09-04  1797  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1798  static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1799  				    unsigned long arg)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1800  {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1801  	int ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1802  	struct uffdio_writeprotect uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1803  	struct uffdio_writeprotect __user *user_uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1804  	struct userfaultfd_wake_range range;
23080e2783ba45 Peter Xu         2020-04-06  1805  	bool mode_wp, mode_dontwake;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1806  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1807  	if (READ_ONCE(ctx->mmap_changing))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1808  		return -EAGAIN;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1809  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1810  	user_uffdio_wp = (struct uffdio_writeprotect __user *) arg;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1811  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1812  	if (copy_from_user(&uffdio_wp, user_uffdio_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1813  			   sizeof(struct uffdio_writeprotect)))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1814  		return -EFAULT;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1815  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06 @1816  	ret = validate_range(ctx->mm, &uffdio_wp.range.start,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1817  			     uffdio_wp.range.len);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1818  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1819  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1820  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1821  	if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1822  			       UFFDIO_WRITEPROTECT_MODE_WP))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1823  		return -EINVAL;
23080e2783ba45 Peter Xu         2020-04-06  1824  
23080e2783ba45 Peter Xu         2020-04-06  1825  	mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
23080e2783ba45 Peter Xu         2020-04-06  1826  	mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
23080e2783ba45 Peter Xu         2020-04-06  1827  
23080e2783ba45 Peter Xu         2020-04-06  1828  	if (mode_wp && mode_dontwake)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1829  		return -EINVAL;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1830  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1831  	ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
23080e2783ba45 Peter Xu         2020-04-06  1832  				  uffdio_wp.range.len, mode_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1833  				  &ctx->mmap_changing);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1834  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1835  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1836  
23080e2783ba45 Peter Xu         2020-04-06  1837  	if (!mode_wp && !mode_dontwake) {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1838  		range.start = uffdio_wp.range.start;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1839  		range.len = uffdio_wp.range.len;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1840  		wake_userfault(ctx, &range);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1841  	}
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1842  	return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1843  }
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1844  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot July 3, 2021, 5:02 a.m. UTC | #3 
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on kselftest/next linus/master v5.13 next-20210701]
[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/Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: openrisc-randconfig-c004-20210702 (attached as .config)
compiler: or1k-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
        # https://github.com/0day-ci/linux/commit/42177546697fa573571799dc11ecd12a65449886
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
        git checkout 42177546697fa573571799dc11ecd12a65449886
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

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 >>):

   fs/userfaultfd.c: In function 'userfaultfd_writeprotect':
>> fs/userfaultfd.c:1816:32: warning: passing argument 2 of 'validate_range' makes integer from pointer without a cast [-Wint-conversion]
    1816 |  ret = validate_range(ctx->mm, &uffdio_wp.range.start,
         |                                ^~~~~~~~~~~~~~~~~~~~~~
         |                                |
         |                                __u64 * {aka long long unsigned int *}
   fs/userfaultfd.c:1239:14: note: expected '__u64' {aka 'long long unsigned int'} but argument is of type '__u64 *' {aka 'long long unsigned int *'}
    1239 |        __u64 start, __u64 len)
         |        ~~~~~~^~~~~
   fs/userfaultfd.c: In function 'userfaultfd_continue':
   fs/userfaultfd.c:1864:32: warning: passing argument 2 of 'validate_range' makes integer from pointer without a cast [-Wint-conversion]
    1864 |  ret = validate_range(ctx->mm, &uffdio_continue.range.start,
         |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                |
         |                                __u64 * {aka long long unsigned int *}
   fs/userfaultfd.c:1239:14: note: expected '__u64' {aka 'long long unsigned int'} but argument is of type '__u64 *' {aka 'long long unsigned int *'}
    1239 |        __u64 start, __u64 len)
         |        ~~~~~~^~~~~


vim +/validate_range +1816 fs/userfaultfd.c

ad465cae96b456 Andrea Arcangeli 2015-09-04  1797  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1798  static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1799  				    unsigned long arg)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1800  {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1801  	int ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1802  	struct uffdio_writeprotect uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1803  	struct uffdio_writeprotect __user *user_uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1804  	struct userfaultfd_wake_range range;
23080e2783ba45 Peter Xu         2020-04-06  1805  	bool mode_wp, mode_dontwake;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1806  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1807  	if (READ_ONCE(ctx->mmap_changing))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1808  		return -EAGAIN;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1809  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1810  	user_uffdio_wp = (struct uffdio_writeprotect __user *) arg;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1811  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1812  	if (copy_from_user(&uffdio_wp, user_uffdio_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1813  			   sizeof(struct uffdio_writeprotect)))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1814  		return -EFAULT;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1815  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06 @1816  	ret = validate_range(ctx->mm, &uffdio_wp.range.start,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1817  			     uffdio_wp.range.len);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1818  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1819  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1820  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1821  	if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1822  			       UFFDIO_WRITEPROTECT_MODE_WP))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1823  		return -EINVAL;
23080e2783ba45 Peter Xu         2020-04-06  1824  
23080e2783ba45 Peter Xu         2020-04-06  1825  	mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
23080e2783ba45 Peter Xu         2020-04-06  1826  	mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
23080e2783ba45 Peter Xu         2020-04-06  1827  
23080e2783ba45 Peter Xu         2020-04-06  1828  	if (mode_wp && mode_dontwake)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1829  		return -EINVAL;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1830  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1831  	ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
23080e2783ba45 Peter Xu         2020-04-06  1832  				  uffdio_wp.range.len, mode_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1833  				  &ctx->mmap_changing);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1834  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1835  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1836  
23080e2783ba45 Peter Xu         2020-04-06  1837  	if (!mode_wp && !mode_dontwake) {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1838  		range.start = uffdio_wp.range.start;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1839  		range.len = uffdio_wp.range.len;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1840  		wake_userfault(ctx, &range);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1841  	}
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1842  	return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1843  }
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1844  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot July 3, 2021, 9:05 a.m. UTC | #4 
Hi Peter,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on kselftest/next v5.13]
[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/Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: i386-randconfig-s002-20210702 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/42177546697fa573571799dc11ecd12a65449886
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Collingbourne/userfaultfd-do-not-untag-user-pointers/20210703-065801
        git checkout 42177546697fa573571799dc11ecd12a65449886
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash

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


sparse warnings: (new ones prefixed by >>)
   fs/userfaultfd.c:1816:40: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned long long [usertype] start @@     got unsigned long long * @@
   fs/userfaultfd.c:1816:40: sparse:     expected unsigned long long [usertype] start
   fs/userfaultfd.c:1816:40: sparse:     got unsigned long long *
   fs/userfaultfd.c:1864:40: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected unsigned long long [usertype] start @@     got unsigned long long * @@
   fs/userfaultfd.c:1864:40: sparse:     expected unsigned long long [usertype] start
   fs/userfaultfd.c:1864:40: sparse:     got unsigned long long *
>> fs/userfaultfd.c:1816:40: sparse: sparse: non size-preserving pointer to integer cast
   fs/userfaultfd.c:1864:40: sparse: sparse: non size-preserving pointer to integer cast

vim +1816 fs/userfaultfd.c

ad465cae96b456 Andrea Arcangeli 2015-09-04  1797  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1798  static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1799  				    unsigned long arg)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1800  {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1801  	int ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1802  	struct uffdio_writeprotect uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1803  	struct uffdio_writeprotect __user *user_uffdio_wp;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1804  	struct userfaultfd_wake_range range;
23080e2783ba45 Peter Xu         2020-04-06  1805  	bool mode_wp, mode_dontwake;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1806  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1807  	if (READ_ONCE(ctx->mmap_changing))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1808  		return -EAGAIN;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1809  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1810  	user_uffdio_wp = (struct uffdio_writeprotect __user *) arg;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1811  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1812  	if (copy_from_user(&uffdio_wp, user_uffdio_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1813  			   sizeof(struct uffdio_writeprotect)))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1814  		return -EFAULT;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1815  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06 @1816  	ret = validate_range(ctx->mm, &uffdio_wp.range.start,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1817  			     uffdio_wp.range.len);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1818  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1819  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1820  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1821  	if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1822  			       UFFDIO_WRITEPROTECT_MODE_WP))
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1823  		return -EINVAL;
23080e2783ba45 Peter Xu         2020-04-06  1824  
23080e2783ba45 Peter Xu         2020-04-06  1825  	mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
23080e2783ba45 Peter Xu         2020-04-06  1826  	mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
23080e2783ba45 Peter Xu         2020-04-06  1827  
23080e2783ba45 Peter Xu         2020-04-06  1828  	if (mode_wp && mode_dontwake)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1829  		return -EINVAL;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1830  
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1831  	ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
23080e2783ba45 Peter Xu         2020-04-06  1832  				  uffdio_wp.range.len, mode_wp,
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1833  				  &ctx->mmap_changing);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1834  	if (ret)
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1835  		return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1836  
23080e2783ba45 Peter Xu         2020-04-06  1837  	if (!mode_wp && !mode_dontwake) {
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1838  		range.start = uffdio_wp.range.start;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1839  		range.len = uffdio_wp.range.len;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1840  		wake_userfault(ctx, &range);
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1841  	}
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1842  	return ret;
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1843  }
63b2d4174c4ad1 Andrea Arcangeli 2020-04-06  1844  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Andrey Konovalov July 4, 2021, 3:39 p.m. UTC | #5 
On Sat, Jul 3, 2021 at 12:57 AM Peter Collingbourne <pcc@google.com> wrote:
>
> If a user program uses userfaultfd on ranges of heap memory, it may
> end up passing a tagged pointer to the kernel in the range.start
> field of the UFFDIO_REGISTER ioctl. This can happen when using an
> MTE-capable allocator, or on Android if using the Tagged Pointers
> feature for MTE readiness [1].
>
> When a fault subsequently occurs, the tag is stripped from the fault
> address returned to the application in the fault.address field
> of struct uffd_msg. However, from the application's perspective,
> the tagged address *is* the memory address, so if the application
> is unaware of memory tags, it may get confused by receiving an
> address that is, from its point of view, outside of the bounds of the
> allocation. We observed this behavior in the kselftest for userfaultfd
> [2] but other applications could have the same problem.
>
> Address this by not untagging pointers passed to the userfaultfd
> ioctls. Instead, let the system call fail. This will provide an
> early indication of problems with tag-unaware userspace code instead
> of letting the code get confused later, and is consistent with how
> we decided to handle brk/mmap/mremap in commit dcde237319e6 ("mm:
> Avoid creating virtual address aliases in brk()/mmap()/mremap()"),
> as well as being consistent with the existing tagged address ABI
> documentation relating to how ioctl arguments are handled.
>
> The code change is a revert of commit 7d0325749a6c ("userfaultfd:
> untag user pointers").
>
> [1] https://source.android.com/devices/tech/debug/tagged-pointers
> [2] tools/testing/selftests/vm/userfaultfd.c
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/I761aa9f0344454c482b83fcfcce547db0a25501b
> Fixes: 63f0c6037965 ("arm64: Introduce prctl() options to control the tagged user addresses ABI")
> Cc: <stable@vger.kernel.org> # 5.4
> ---
>  Documentation/arm64/tagged-address-abi.rst | 25 +++++++++++++++-------
>  fs/userfaultfd.c                           | 22 +++++++++----------
>  2 files changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
> index 459e6b66ff68..737f9d8565a2 100644
> --- a/Documentation/arm64/tagged-address-abi.rst
> +++ b/Documentation/arm64/tagged-address-abi.rst
> @@ -45,14 +45,23 @@ how the user addresses are used by the kernel:
>
>  1. User addresses not accessed by the kernel but used for address space
>     management (e.g. ``mprotect()``, ``madvise()``). The use of valid
> -   tagged pointers in this context is allowed with the exception of
> -   ``brk()``, ``mmap()`` and the ``new_address`` argument to
> -   ``mremap()`` as these have the potential to alias with existing
> -   user addresses.
> -
> -   NOTE: This behaviour changed in v5.6 and so some earlier kernels may
> -   incorrectly accept valid tagged pointers for the ``brk()``,
> -   ``mmap()`` and ``mremap()`` system calls.
> +   tagged pointers in this context is allowed with these exceptions:
> +
> +   - ``brk()``, ``mmap()`` and the ``new_address`` argument to
> +     ``mremap()`` as these have the potential to alias with existing
> +      user addresses.
> +
> +     NOTE: This behaviour changed in v5.6 and so some earlier kernels may
> +     incorrectly accept valid tagged pointers for the ``brk()``,
> +     ``mmap()`` and ``mremap()`` system calls.
> +
> +   - The ``range.start`` argument to the ``UFFDIO_REGISTER`` ``ioctl()``
> +     used on a file descriptor obtained from ``userfaultfd()``, as
> +     fault addresses subsequently obtained by reading the file descriptor
> +     will be untagged, which may otherwise confuse tag-unaware programs.
> +
> +     NOTE: This behaviour changed in v5.14 and so some earlier kernels may
> +     incorrectly accept valid tagged pointers for this system call.
>
>  2. User addresses accessed by the kernel (e.g. ``write()``). This ABI
>     relaxation is disabled by default and the application thread needs to
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index dd7a6c62b56f..7613efe002c1 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1236,23 +1236,21 @@ static __always_inline void wake_userfault(struct userfaultfd_ctx *ctx,
>  }
>
>  static __always_inline int validate_range(struct mm_struct *mm,
> -                                         __u64 *start, __u64 len)
> +                                         __u64 start, __u64 len)
>  {
>         __u64 task_size = mm->task_size;
>
> -       *start = untagged_addr(*start);
> -
> -       if (*start & ~PAGE_MASK)
> +       if (start & ~PAGE_MASK)
>                 return -EINVAL;
>         if (len & ~PAGE_MASK)
>                 return -EINVAL;
>         if (!len)
>                 return -EINVAL;
> -       if (*start < mmap_min_addr)
> +       if (start < mmap_min_addr)
>                 return -EINVAL;
> -       if (*start >= task_size)
> +       if (start >= task_size)
>                 return -EINVAL;
> -       if (len > task_size - *start)
> +       if (len > task_size - start)
>                 return -EINVAL;
>         return 0;
>  }
> @@ -1313,7 +1311,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>                 vm_flags |= VM_UFFD_MINOR;
>         }
>
> -       ret = validate_range(mm, &uffdio_register.range.start,
> +       ret = validate_range(mm, uffdio_register.range.start,
>                              uffdio_register.range.len);
>         if (ret)
>                 goto out;
> @@ -1519,7 +1517,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>         if (copy_from_user(&uffdio_unregister, buf, sizeof(uffdio_unregister)))
>                 goto out;
>
> -       ret = validate_range(mm, &uffdio_unregister.start,
> +       ret = validate_range(mm, uffdio_unregister.start,
>                              uffdio_unregister.len);
>         if (ret)
>                 goto out;
> @@ -1668,7 +1666,7 @@ static int userfaultfd_wake(struct userfaultfd_ctx *ctx,
>         if (copy_from_user(&uffdio_wake, buf, sizeof(uffdio_wake)))
>                 goto out;
>
> -       ret = validate_range(ctx->mm, &uffdio_wake.start, uffdio_wake.len);
> +       ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len);
>         if (ret)
>                 goto out;
>
> @@ -1708,7 +1706,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
>                            sizeof(uffdio_copy)-sizeof(__s64)))
>                 goto out;
>
> -       ret = validate_range(ctx->mm, &uffdio_copy.dst, uffdio_copy.len);
> +       ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
>         if (ret)
>                 goto out;
>         /*
> @@ -1765,7 +1763,7 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
>                            sizeof(uffdio_zeropage)-sizeof(__s64)))
>                 goto out;
>
> -       ret = validate_range(ctx->mm, &uffdio_zeropage.range.start,
> +       ret = validate_range(ctx->mm, uffdio_zeropage.range.start,
>                              uffdio_zeropage.range.len);
>         if (ret)
>                 goto out;
> --
> 2.32.0.93.g670b81a890-goog
>

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
diff mbox series

Patch

diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
index 459e6b66ff68..737f9d8565a2 100644
--- a/Documentation/arm64/tagged-address-abi.rst
+++ b/Documentation/arm64/tagged-address-abi.rst
@@ -45,14 +45,23 @@  how the user addresses are used by the kernel:
 
 1. User addresses not accessed by the kernel but used for address space
    management (e.g. ``mprotect()``, ``madvise()``). The use of valid
-   tagged pointers in this context is allowed with the exception of
-   ``brk()``, ``mmap()`` and the ``new_address`` argument to
-   ``mremap()`` as these have the potential to alias with existing
-   user addresses.
-
-   NOTE: This behaviour changed in v5.6 and so some earlier kernels may
-   incorrectly accept valid tagged pointers for the ``brk()``,
-   ``mmap()`` and ``mremap()`` system calls.
+   tagged pointers in this context is allowed with these exceptions:
+
+   - ``brk()``, ``mmap()`` and the ``new_address`` argument to
+     ``mremap()`` as these have the potential to alias with existing
+      user addresses.
+
+     NOTE: This behaviour changed in v5.6 and so some earlier kernels may
+     incorrectly accept valid tagged pointers for the ``brk()``,
+     ``mmap()`` and ``mremap()`` system calls.
+
+   - The ``range.start`` argument to the ``UFFDIO_REGISTER`` ``ioctl()``
+     used on a file descriptor obtained from ``userfaultfd()``, as
+     fault addresses subsequently obtained by reading the file descriptor
+     will be untagged, which may otherwise confuse tag-unaware programs.
+
+     NOTE: This behaviour changed in v5.14 and so some earlier kernels may
+     incorrectly accept valid tagged pointers for this system call.
 
 2. User addresses accessed by the kernel (e.g. ``write()``). This ABI
    relaxation is disabled by default and the application thread needs to
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index dd7a6c62b56f..7613efe002c1 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1236,23 +1236,21 @@  static __always_inline void wake_userfault(struct userfaultfd_ctx *ctx,
 }
 
 static __always_inline int validate_range(struct mm_struct *mm,
-					  __u64 *start, __u64 len)
+					  __u64 start, __u64 len)
 {
 	__u64 task_size = mm->task_size;
 
-	*start = untagged_addr(*start);
-
-	if (*start & ~PAGE_MASK)
+	if (start & ~PAGE_MASK)
 		return -EINVAL;
 	if (len & ~PAGE_MASK)
 		return -EINVAL;
 	if (!len)
 		return -EINVAL;
-	if (*start < mmap_min_addr)
+	if (start < mmap_min_addr)
 		return -EINVAL;
-	if (*start >= task_size)
+	if (start >= task_size)
 		return -EINVAL;
-	if (len > task_size - *start)
+	if (len > task_size - start)
 		return -EINVAL;
 	return 0;
 }
@@ -1313,7 +1311,7 @@  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
 		vm_flags |= VM_UFFD_MINOR;
 	}
 
-	ret = validate_range(mm, &uffdio_register.range.start,
+	ret = validate_range(mm, uffdio_register.range.start,
 			     uffdio_register.range.len);
 	if (ret)
 		goto out;
@@ -1519,7 +1517,7 @@  static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 	if (copy_from_user(&uffdio_unregister, buf, sizeof(uffdio_unregister)))
 		goto out;
 
-	ret = validate_range(mm, &uffdio_unregister.start,
+	ret = validate_range(mm, uffdio_unregister.start,
 			     uffdio_unregister.len);
 	if (ret)
 		goto out;
@@ -1668,7 +1666,7 @@  static int userfaultfd_wake(struct userfaultfd_ctx *ctx,
 	if (copy_from_user(&uffdio_wake, buf, sizeof(uffdio_wake)))
 		goto out;
 
-	ret = validate_range(ctx->mm, &uffdio_wake.start, uffdio_wake.len);
+	ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len);
 	if (ret)
 		goto out;
 
@@ -1708,7 +1706,7 @@  static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 			   sizeof(uffdio_copy)-sizeof(__s64)))
 		goto out;
 
-	ret = validate_range(ctx->mm, &uffdio_copy.dst, uffdio_copy.len);
+	ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
 	if (ret)
 		goto out;
 	/*
@@ -1765,7 +1763,7 @@  static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
 			   sizeof(uffdio_zeropage)-sizeof(__s64)))
 		goto out;
 
-	ret = validate_range(ctx->mm, &uffdio_zeropage.range.start,
+	ret = validate_range(ctx->mm, uffdio_zeropage.range.start,
 			     uffdio_zeropage.range.len);
 	if (ret)
 		goto out;