Re: [PATCH next V3] proc: support file->f_pos checking in mem_lseek

Re: [PATCH next V3] proc: support file->f_pos checking in mem_lseek

[Al Viro]

On Fri, Nov 10, 2023 at 11:19:28PM +0800, Zizhi Wo wrote:
> From: WoZ1zh1 <wozizhi@huawei.com>
> 
> In mem_lseek, file->f_pos may overflow. And it's not a problem that
> mem_open set file mode with FMODE_UNSIGNED_OFFSET(memory_lseek). However,
> another file use mem_lseek do lseek can have not FMODE_UNSIGNED_OFFSET
> (kpageflags_proc_ops/proc_pagemap_operations...), so in order to prevent
> file->f_pos updated to an abnormal number, fix it by checking overflow and
> FMODE_UNSIGNED_OFFSET.

1) How is that different from the previous version?
2) Would FMODE_UNSIGNED_OFFSET be wrong for those files?  If not, why not?

Re: [PATCH next V3] proc: support file->f_pos checking in mem_lseek

[Oleg Nesterov]

On 11/10, Zizhi Wo wrote:
>
> From: WoZ1zh1 <wozizhi@huawei.com>
>
> In mem_lseek, file->f_pos may overflow. And it's not a problem that
> mem_open set file mode with FMODE_UNSIGNED_OFFSET(memory_lseek). However,
> another file use mem_lseek do lseek can have not FMODE_UNSIGNED_OFFSET
> (kpageflags_proc_ops/proc_pagemap_operations...), so in order to prevent
> file->f_pos updated to an abnormal number, fix it by checking overflow and
> FMODE_UNSIGNED_OFFSET.

I am wondering if we can do something like the patch below instead...

but I agree that the "proc_lseek == mem_lseek" in proc_reg_open()
looks ugly.

Oleg.

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 532dc9d240f7..af7e6b1e17fe 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -496,6 +496,8 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 
 	if (!pde->proc_ops->proc_lseek)
 		file->f_mode &= ~FMODE_LSEEK;
+	else if (pde->proc_ops->proc_lseek == mem_lseek)
+		file->f_mode |= FMODE_UNSIGNED_OFFSET;
 
 	if (pde_is_permanent(pde)) {
 		open = pde->proc_ops->proc_open;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3dd5be96691b..729b28ad1a96 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1748,7 +1748,9 @@ static int pagemap_open(struct inode *inode, struct file *file)
 	mm = proc_mem_open(inode, PTRACE_MODE_READ);
 	if (IS_ERR(mm))
 		return PTR_ERR(mm);
+
 	file->private_data = mm;
+	file->f_mode |= FMODE_UNSIGNED_OFFSET;
 	return 0;
 }
 

[PATCH next V3] proc: support file->f_pos checking in mem_lseek

[Zizhi Wo]

From: WoZ1zh1 <wozizhi@huawei.com>

In mem_lseek, file->f_pos may overflow. And it's not a problem that
mem_open set file mode with FMODE_UNSIGNED_OFFSET(memory_lseek). However,
another file use mem_lseek do lseek can have not FMODE_UNSIGNED_OFFSET
(kpageflags_proc_ops/proc_pagemap_operations...), so in order to prevent
file->f_pos updated to an abnormal number, fix it by checking overflow and
FMODE_UNSIGNED_OFFSET.

Signed-off-by: WoZ1zh1 <wozizhi@huawei.com>
---
 fs/proc/base.c     | 31 +++++++++++++++++++++++--------
 fs/read_write.c    |  5 -----
 include/linux/fs.h |  5 ++++-
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index dd31e3b6bf77..20f5fcf6b73e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -903,18 +903,33 @@ static ssize_t mem_write(struct file *file, const char __user *buf,
 
 loff_t mem_lseek(struct file *file, loff_t offset, int orig)
 {
+	loff_t ret = 0;
+
+	spin_lock(&file->f_lock);
 	switch (orig) {
-	case 0:
-		file->f_pos = offset;
-		break;
-	case 1:
-		file->f_pos += offset;
+	case SEEK_CUR:
+		offset += file->f_pos;
+		fallthrough;
+	case SEEK_SET:
+		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
+		if ((unsigned long long)offset >= -MAX_ERRNO)
+			ret = -EOVERFLOW;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
-	force_successful_syscall_return();
-	return file->f_pos;
+	if (!ret) {
+		if (offset < 0 && !(unsigned_offsets(file))) {
+			ret = -EINVAL;
+		} else {
+			file->f_pos = offset;
+			ret = file->f_pos;
+			force_successful_syscall_return();
+		}
+	}
+
+	spin_unlock(&file->f_lock);
+	return ret;
 }
 
 static int mem_release(struct inode *inode, struct file *file)
diff --git a/fs/read_write.c b/fs/read_write.c
index 4771701c896b..2f456d5a1df5 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -34,11 +34,6 @@ const struct file_operations generic_ro_fops = {
 
 EXPORT_SYMBOL(generic_ro_fops);
 
-static inline bool unsigned_offsets(struct file *file)
-{
-	return file->f_mode & FMODE_UNSIGNED_OFFSET;
-}
-
 /**
  * vfs_setpos - update the file offset for lseek
  * @file:	file structure in question
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..dde0756d2350 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2994,7 +2994,10 @@ extern ssize_t iter_file_splice_write(struct pipe_inode_info *,
 extern long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
 		loff_t *opos, size_t len, unsigned int flags);
 
-
+static inline bool unsigned_offsets(struct file *file)
+{
+	return file->f_mode & FMODE_UNSIGNED_OFFSET;
+}
 extern void
 file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);
 extern loff_t noop_llseek(struct file *file, loff_t offset, int whence);
-- 
2.39.2