[bpf-next,v4,1/3] mm: add copy_remote_vm_str

Commit Message

Jordan Rome Jan. 26, 2025, 12:41 p.m. UTC

Similar to `access_process_vm` but specific to strings.
Also chunks reads by page and utilizes `strscpy`
for handling null termination.

Signed-off-by: Jordan Rome <[e-mail]>
---
 include/linux/mm.h |   3 ++
 mm/memory.c        | 118 +++++++++++++++++++++++++++++++++++++++++++++
 mm/nommu.c         |  66 +++++++++++++++++++++++++
 3 files changed, 187 insertions(+)

--
2.43.5

Comments

kernel test robot Jan. 26, 2025, 2:08 p.m. UTC | #1

Hi Jordan,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master linus/master v6.13 next-20250124]
[cannot apply to akpm-mm/mm-everything]
[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/Jordan-Rome/bpf-Add-bpf_copy_from_user_task_str-kfunc/20250126-204439
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20250126124147.3154108-1-linux%40jordanrome.com
patch subject: [bpf-next v4 1/3] mm: add copy_remote_vm_str
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20250126/[e-mail]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250126/[e-mail]/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 <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

>> mm/nommu.c:1717:2: error: use of undeclared identifier 'vma'
    1717 |         vma = find_vma(mm, addr);
         |         ^
   mm/nommu.c:1718:6: error: use of undeclared identifier 'vma'; did you mean 'vmap'?
    1718 |         if (vma) {
         |             ^~~
         |             vmap
   mm/nommu.c:303:15: note: 'vmap' declared here
     303 | EXPORT_SYMBOL(vmap);
         |               ^
   mm/nommu.c:1720:21: error: use of undeclared identifier 'vma'
    1720 |                 if (addr + len >= vma->vm_end)
         |                                   ^
   mm/nommu.c:1721:10: error: use of undeclared identifier 'vma'
    1721 |                         len = vma->vm_end - addr;
         |                               ^
   mm/nommu.c:1724:7: error: use of undeclared identifier 'vma'
    1724 |                 if (vma->vm_flags & VM_MAYREAD) {
         |                     ^
>> mm/nommu.c:1725:23: error: incompatible integer to pointer conversion passing 'unsigned long' to parameter of type 'const char *' [-Wint-conversion]
    1725 |                         ret = strscpy(buf, addr, len);
         |                                            ^~~~
   include/linux/string.h:113:55: note: expanded from macro 'strscpy'
     113 |         CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
         |                                                              ^~~
   include/linux/string.h:82:21: note: expanded from macro '__strscpy1'
      82 |         sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
         |                            ^~~
   include/linux/string.h:72:43: note: passing argument to parameter here
      72 | ssize_t sized_strscpy(char *, const char *, size_t);
         |                                           ^
   6 errors generated.


vim +/vma +1717 mm/nommu.c

  1703	
  1704	/*
  1705	 * Copy a string from another process's address space as given in mm.
  1706	 * If there is any error return -EFAULT.
  1707	 */
  1708	static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
  1709				      void *buf, int len)
  1710	{
  1711		int ret;
  1712	
  1713		if (mmap_read_lock_killable(mm))
  1714			return -EFAULT;
  1715	
  1716		/* the access must start within one of the target process's mappings */
> 1717		vma = find_vma(mm, addr);
  1718		if (vma) {
  1719			/* don't overrun this mapping */
> 1720			if (addr + len >= vma->vm_end)
  1721				len = vma->vm_end - addr;
  1722	
  1723			/* only read mappings where it is permitted */
  1724			if (vma->vm_flags & VM_MAYREAD) {
> 1725				ret = strscpy(buf, addr, len);
  1726				if (ret < 0)
  1727					ret = len - 1;
  1728			} else {
  1729				ret = -EFAULT;
  1730			}
  1731		} else {
  1732			ret = -EFAULT;
  1733		}
  1734	
  1735		mmap_read_unlock(mm);
  1736		return ret;
  1737	}
  1738

kernel test robot Jan. 26, 2025, 2:28 p.m. UTC | #2

Hi Jordan,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master linus/master v6.13 next-20250124]
[cannot apply to akpm-mm/mm-everything]
[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/Jordan-Rome/bpf-Add-bpf_copy_from_user_task_str-kfunc/20250126-204439
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20250126124147.3154108-1-linux%40jordanrome.com
patch subject: [bpf-next v4 1/3] mm: add copy_remote_vm_str
config: arm-randconfig-001-20250126 (https://download.01.org/0day-ci/archive/20250126/[e-mail]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250126/[e-mail]/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 <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

   mm/nommu.c: In function '__copy_remote_vm_str':
>> mm/nommu.c:1717:9: error: 'vma' undeclared (first use in this function); did you mean 'vmap'?
    1717 |         vma = find_vma(mm, addr);
         |         ^~~
         |         vmap
   mm/nommu.c:1717:9: note: each undeclared identifier is reported only once for each function it appears in
   In file included from include/linux/bitmap.h:13,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:63,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/mm.h:7,
                    from mm/nommu.c:20:
>> mm/nommu.c:1725:44: error: passing argument 2 of 'sized_strscpy' makes pointer from integer without a cast [-Wint-conversion]
    1725 |                         ret = strscpy(buf, addr, len);
         |                                            ^~~~
         |                                            |
         |                                            long unsigned int
   include/linux/string.h:82:28: note: in definition of macro '__strscpy1'
      82 |         sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
         |                            ^~~
   mm/nommu.c:1725:31: note: in expansion of macro 'strscpy'
    1725 |                         ret = strscpy(buf, addr, len);
         |                               ^~~~~~~
   include/linux/string.h:72:31: note: expected 'const char *' but argument is of type 'long unsigned int'
      72 | ssize_t sized_strscpy(char *, const char *, size_t);
         |                               ^~~~~~~~~~~~


vim +1717 mm/nommu.c

  1703	
  1704	/*
  1705	 * Copy a string from another process's address space as given in mm.
  1706	 * If there is any error return -EFAULT.
  1707	 */
  1708	static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
  1709				      void *buf, int len)
  1710	{
  1711		int ret;
  1712	
  1713		if (mmap_read_lock_killable(mm))
  1714			return -EFAULT;
  1715	
  1716		/* the access must start within one of the target process's mappings */
> 1717		vma = find_vma(mm, addr);
  1718		if (vma) {
  1719			/* don't overrun this mapping */
  1720			if (addr + len >= vma->vm_end)
  1721				len = vma->vm_end - addr;
  1722	
  1723			/* only read mappings where it is permitted */
  1724			if (vma->vm_flags & VM_MAYREAD) {
> 1725				ret = strscpy(buf, addr, len);
  1726				if (ret < 0)
  1727					ret = len - 1;
  1728			} else {
  1729				ret = -EFAULT;
  1730			}
  1731		} else {
  1732			ret = -EFAULT;
  1733		}
  1734	
  1735		mmap_read_unlock(mm);
  1736		return ret;
  1737	}
  1738

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index f02925447e59..f3a05b3eb2f2 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2485,6 +2485,9 @@  extern int access_process_vm(struct task_struct *tsk, unsigned long addr,
 extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
 		void *buf, int len, unsigned int gup_flags);

+extern int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
+		void *buf, int len, unsigned int gup_flags);
+
 long get_user_pages_remote(struct mm_struct *mm,
 			   unsigned long start, unsigned long nr_pages,
 			   unsigned int gup_flags, struct page **pages,
diff --git a/mm/memory.c b/mm/memory.c
index 398c031be9ba..e1ed5095b258 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6714,6 +6714,124 @@  int access_process_vm(struct task_struct *tsk, unsigned long addr,
 }
 EXPORT_SYMBOL_GPL(access_process_vm);

+/*
+ * Copy a string from another process's address space as given in mm.
+ * If there is any error return -EFAULT.
+ */
+static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
+			      void *buf, int len, unsigned int gup_flags)
+{
+	void *old_buf = buf;
+	int err = 0;
+
+	if (mmap_read_lock_killable(mm))
+		return -EFAULT;
+
+	/* Untag the address before looking up the VMA */
+	addr = untagged_addr_remote(mm, addr);
+
+	/* Avoid triggering the temporary warning in __get_user_pages */
+	if (!vma_lookup(mm, addr)) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	while (len) {
+		int bytes, offset, retval;
+		void *maddr;
+		struct page *page;
+		struct vm_area_struct *vma = NULL;
+
+		page = get_user_page_vma_remote(mm, addr, gup_flags, &vma);
+
+		if (IS_ERR(page)) {
+			/*
+			 * Treat as a total failure for now until we decide how
+			 * to handle the CONFIG_HAVE_IOREMAP_PROT case and
+			 * stack expansion.
+			 */
+			err = -EFAULT;
+			goto out;
+		}
+
+		bytes = len;
+		offset = addr & (PAGE_SIZE - 1);
+		if (bytes > PAGE_SIZE - offset)
+			bytes = PAGE_SIZE - offset;
+
+		maddr = kmap_local_page(page);
+		retval = strscpy(buf, maddr + offset, bytes);
+		unmap_and_put_page(page, maddr);
+
+		if (retval > -1) {
+			/* Found the end of the string */
+			buf += retval;
+			goto out;
+		}
+
+		retval = bytes - 1;
+		buf += retval;
+
+		if (bytes == len)
+			goto out;
+
+		/*
+		 * Because strscpy always NUL terminates we need to
+		 * copy the last byte in the page if we are going to
+		 * load more pages
+		 */
+		addr += retval;
+		len -= retval;
+		copy_from_user_page(vma,
+				page,
+				addr,
+				buf,
+				maddr + (PAGE_SIZE - 1),
+				1);
+		len -= 1;
+		buf += 1;
+		addr += 1;
+	}
+
+out:
+	mmap_read_unlock(mm);
+	if (err)
+		return err;
+
+	return buf - old_buf;
+}
+
+/**
+ * copy_remote_vm_str - copy a string from another process's address space.
+ * @tsk:	the task of the target address space
+ * @addr:	start address to read from
+ * @buf:	destination buffer
+ * @len:	number of bytes to copy
+ * @gup_flags:	flags modifying lookup behaviour
+ *
+ * The caller must hold a reference on @mm.
+ *
+ * Return: number of bytes copied from @addr (source) to @buf (destination);
+ * not including the trailing NUL. On any error, return -EFAULT.
+ */
+int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
+		void *buf, int len, unsigned int gup_flags)
+{
+	struct mm_struct *mm;
+	int ret;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return -EFAULT;
+
+	ret = __copy_remote_vm_str(mm, addr, buf, len, gup_flags);
+
+	mmput(mm);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(copy_remote_vm_str);
+
 /*
  * Print the name of a VMA.
  */
diff --git a/mm/nommu.c b/mm/nommu.c
index 9cb6e99215e2..480ddf50dec1 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1701,6 +1701,72 @@  int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
 }
 EXPORT_SYMBOL_GPL(access_process_vm);

+/*
+ * Copy a string from another process's address space as given in mm.
+ * If there is any error return -EFAULT.
+ */
+static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
+			      void *buf, int len)
+{
+	int ret;
+
+	if (mmap_read_lock_killable(mm))
+		return -EFAULT;
+
+	/* the access must start within one of the target process's mappings */
+	vma = find_vma(mm, addr);
+	if (vma) {
+		/* don't overrun this mapping */
+		if (addr + len >= vma->vm_end)
+			len = vma->vm_end - addr;
+
+		/* only read mappings where it is permitted */
+		if (vma->vm_flags & VM_MAYREAD) {
+			ret = strscpy(buf, addr, len);
+			if (ret < 0)
+				ret = len - 1;
+		} else {
+			ret = -EFAULT;
+		}
+	} else {
+		ret = -EFAULT;
+	}
+
+	mmap_read_unlock(mm);
+	return ret;
+}
+
+/**
+ * copy_remote_vm_str - copy a string from another process's address space.
+ * @tsk:	the task of the target address space
+ * @addr:	start address to read from
+ * @buf:	destination buffer
+ * @len:	number of bytes to copy
+ * @gup_flags:	flags modifying lookup behaviour (unused)
+ *
+ * The caller must hold a reference on @mm.
+ *
+ * Return: number of bytes copied from @addr (source) to @buf (destination);
+ * not including the trailing NUL. On any error, return -EFAULT.
+ */
+int copy_remote_vm_str(struct task_struct *tsk, unsigned long addr,
+		void *buf, int len, unsigned int gup_flags)
+{
+	struct mm_struct *mm;
+	int ret;
+
+	mm = get_task_mm(tsk);
+	if (!mm)
+		return -EFAULT;
+
+	ret = __copy_remote_vm_str(mm, addr, buf, len);
+
+	mmput(mm);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(copy_remote_vm_str);
+
 /**
  * nommu_shrink_inode_mappings - Shrink the shared mappings on an inode
  * @inode: The inode to check