According to the documentation of fcntl, some commands take an int as argument. In practice not all of them enforce this behaviour, as they instead accept a more permissive long and in most cases not even a range check is performed.
An issue could possibly arise from a combination of the handling of the varargs in user space and the ABI rules of the target, which may result in the top bits of an int argument being non-zero.
This issue was originally raised and detailed in the following thread: https://lore.kernel.org/linux-api/Y1%2FDS6uoWP7OSkmd@arm.com/ And was discovered during the porting of Linux to Morello [1].
This series modifies the interested commands so that they explicitly take an int argument. It also propagates this change down to helper and related functions as necessary.
This series is also available on my fork at: https://git.morello-project.org/Sevenarth/linux/-/commits/fcntl-int-handling...
Best regards, Luca Vizzarro
[1] https://git.morello-project.org/morello/kernel/linux
Luca Vizzarro (5): fcntl: Cast commands with int args explicitly fs: Pass argument to fcntl_setlease as int pipe: Pass argument of pipe_fcntl as int memfd: Pass argument of memfd_fcntl as int dnotify: Pass argument of fcntl_dirnotify as int
fs/cifs/cifsfs.c | 2 +- fs/fcntl.c | 29 +++++++++++++++-------------- fs/libfs.c | 2 +- fs/locks.c | 20 ++++++++++---------- fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4file.c | 2 +- fs/nfs/nfs4proc.c | 4 ++-- fs/notify/dnotify/dnotify.c | 4 ++-- fs/pipe.c | 6 +++--- include/linux/dnotify.h | 4 ++-- include/linux/filelock.h | 12 ++++++------ include/linux/fs.h | 6 +++--- include/linux/memfd.h | 4 ++-- include/linux/pipe_fs_i.h | 4 ++-- mm/memfd.c | 6 +----- 15 files changed, 52 insertions(+), 55 deletions(-)
According to the fcntl API specification commands that expect an integer, hence not a pointer, always take an int and not long. In order to avoid access to undefined bits, we should explicitly cast the argument to int.
Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: Jeff Layton jlayton@kernel.org Cc: Chuck Lever chuck.lever@oracle.com Cc: Kevin Brodsky Kevin.Brodsky@arm.com Cc: Vincenzo Frascino Vincenzo.Frascino@arm.com Cc: Szabolcs Nagy Szabolcs.Nagy@arm.com Cc: "Theodore Ts'o" tytso@mit.edu Cc: David Laight David.Laight@ACULAB.com Cc: Mark Rutland Mark.Rutland@arm.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-morello@op-lists.linaro.org Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- fs/fcntl.c | 29 +++++++++++++++-------------- include/linux/fs.h | 2 +- 2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c index b622be119706..e871009f6c88 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -34,7 +34,7 @@
#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
-static int setfl(int fd, struct file * filp, unsigned long arg) +static int setfl(int fd, struct file * filp, unsigned int arg) { struct inode * inode = file_inode(filp); int error = 0; @@ -112,11 +112,11 @@ void __f_setown(struct file *filp, struct pid *pid, enum pid_type type, } EXPORT_SYMBOL(__f_setown);
-int f_setown(struct file *filp, unsigned long arg, int force) +int f_setown(struct file *filp, int who, int force) { enum pid_type type; struct pid *pid = NULL; - int who = arg, ret = 0; + int ret = 0;
type = PIDTYPE_TGID; if (who < 0) { @@ -317,28 +317,29 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, struct file *filp) { void __user *argp = (void __user *)arg; + int argi = (int)arg; struct flock flock; long err = -EINVAL;
switch (cmd) { case F_DUPFD: - err = f_dupfd(arg, filp, 0); + err = f_dupfd(argi, filp, 0); break; case F_DUPFD_CLOEXEC: - err = f_dupfd(arg, filp, O_CLOEXEC); + err = f_dupfd(argi, filp, O_CLOEXEC); break; case F_GETFD: err = get_close_on_exec(fd) ? FD_CLOEXEC : 0; break; case F_SETFD: err = 0; - set_close_on_exec(fd, arg & FD_CLOEXEC); + set_close_on_exec(fd, argi & FD_CLOEXEC); break; case F_GETFL: err = filp->f_flags; break; case F_SETFL: - err = setfl(fd, filp, arg); + err = setfl(fd, filp, argi); break; #if BITS_PER_LONG != 32 /* 32-bit arches must use fcntl64() */ @@ -375,7 +376,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, force_successful_syscall_return(); break; case F_SETOWN: - err = f_setown(filp, arg, 1); + err = f_setown(filp, argi, 1); break; case F_GETOWN_EX: err = f_getown_ex(filp, arg); @@ -391,28 +392,28 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, break; case F_SETSIG: /* arg == 0 restores default behaviour. */ - if (!valid_signal(arg)) { + if (!valid_signal(argi)) { break; } err = 0; - filp->f_owner.signum = arg; + filp->f_owner.signum = argi; break; case F_GETLEASE: err = fcntl_getlease(filp); break; case F_SETLEASE: - err = fcntl_setlease(fd, filp, arg); + err = fcntl_setlease(fd, filp, argi); break; case F_NOTIFY: - err = fcntl_dirnotify(fd, filp, arg); + err = fcntl_dirnotify(fd, filp, argi); break; case F_SETPIPE_SZ: case F_GETPIPE_SZ: - err = pipe_fcntl(filp, cmd, arg); + err = pipe_fcntl(filp, cmd, argi); break; case F_ADD_SEALS: case F_GET_SEALS: - err = memfd_fcntl(filp, cmd, arg); + err = memfd_fcntl(filp, cmd, argi); break; case F_GET_RW_HINT: case F_SET_RW_HINT: diff --git a/include/linux/fs.h b/include/linux/fs.h index c85916e9f7db..8da79822dbba 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1050,7 +1050,7 @@ extern void fasync_free(struct fasync_struct *); extern void kill_fasync(struct fasync_struct **, int, int);
extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force); -extern int f_setown(struct file *filp, unsigned long arg, int force); +extern int f_setown(struct file *filp, int who, int force); extern void f_delown(struct file *filp); extern pid_t f_getown(struct file *filp); extern int send_sigurg(struct fown_struct *fown);
On Fri, Apr 14, 2023 at 04:24:55PM +0100, Luca Vizzarro wrote:
void __user *argp = (void __user *)arg;
- int argi = (int)arg;
Strictly speaking, conversion from unsigned long to int is an undefined behaviour, unless the value fits into the range representable by int ;-)
case F_SETFD: err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
set_close_on_exec(fd, argi & FD_CLOEXEC);
Why?
case F_SETSIG: /* arg == 0 restores default behaviour. */
if (!valid_signal(arg)) {
if (!valid_signal(argi)) {
Why???
break; } err = 0;
filp->f_owner.signum = arg;
filp->f_owner.signum = argi;
These two are clearly bogus and I'd like to see more details on the series rationale, please.
On Fri, Apr 14, 2023 at 04:46:31PM +0100, Al Viro wrote:
On Fri, Apr 14, 2023 at 04:24:55PM +0100, Luca Vizzarro wrote:
void __user *argp = (void __user *)arg;
- int argi = (int)arg;
Strictly speaking, conversion from unsigned long to int is an undefined behaviour, unless the value fits into the range representable by int ;-)
case F_SETFD: err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
set_close_on_exec(fd, argi & FD_CLOEXEC);Why?
case F_SETSIG: /* arg == 0 restores default behaviour. */
if (!valid_signal(arg)) {
if (!valid_signal(argi)) {Why???
break; } err = 0;
filp->f_owner.signum = arg;
filp->f_owner.signum = argi;These two are clearly bogus and I'd like to see more details on the series rationale, please.
I agree the first isn't necessary, but I don't think the second is bogus, since valid_signal() takes an unsigned long and the man page for F_SETSIG says that the argument is an int:
https://man7.org/linux/man-pages/man2/fcntl.2.html
... though arguably that could be a bug in the man page.
The cover letter really should have quoted the description that Szabolcs wote at:
https://lore.kernel.org/linux-api/Y1%2FDS6uoWP7OSkmd@arm.com/
The gist being that where the calling convention leaves narrowing to callees (as is the case on arm64 with our "AAPCS64" calling convention), if the caller passes a type which is narrower than a register, the upper bits of that register may contain junk.
So e.g. for F_SETSIG, if the userspace will try to pass some 32-bit value, leaving bits 63:32 of the argument register containing arbitrary junk. Then here we interprert the value as an unsigned long, considering that junk as part of the argument. Then valid_signal(arg) may end up rejecting the argument due to the junk uper bits, which is surprising to the caller as from its PoV it passed a 32-bit value in the correct way.
So either:
* That's a documentation bug, and userspce needs to treat the agument to F_SETSIG as an unsigned long.
* The kernel needs to narrow the argument to an int (if required by the calling convention) to prevent that.
Does that make sense, or have I missed the point you were making?
Thanks, Mark.
On 14/04/2023 16:46, Al Viro wrote
Why? ... Why??? ... These two are clearly bogus and I'd like to see more details on the series rationale, please.
Mark preceded me with his reply, which is perfectly summarising the whole point of this series. – Thank you Mark!
As for FD_SETFD, yes it's not necessary. The only reason I changed the variable was to denote the argument as being classified as an `int` as per the man page. If I were not to change it, it would have been the only command with an `int` argument not to use `argi`. Therefore it's also for consistency's sake.
Hope this helps.
Best, Luca
The interface for fcntl expects the argument passed for the command F_SETLEASE to be of type int. The current code wrongly treats it as a long. In order to avoid access to undefined bits, we should explicitly cast the argument to int.
Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: Jeff Layton jlayton@kernel.org Cc: Chuck Lever chuck.lever@oracle.com Cc: Trond Myklebust trond.myklebust@hammerspace.com Cc: Anna Schumaker anna@kernel.org Cc: Kevin Brodsky Kevin.Brodsky@arm.com Cc: Vincenzo Frascino Vincenzo.Frascino@arm.com Cc: Szabolcs Nagy Szabolcs.Nagy@arm.com Cc: "Theodore Ts'o" tytso@mit.edu Cc: David Laight David.Laight@ACULAB.com Cc: Mark Rutland Mark.Rutland@arm.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-cifs@vger.kernel.org Cc: linux-nfs@vger.kernel.org Cc: linux-morello@op-lists.linaro.org Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- fs/cifs/cifsfs.c | 2 +- fs/libfs.c | 2 +- fs/locks.c | 20 ++++++++++---------- fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4file.c | 2 +- fs/nfs/nfs4proc.c | 4 ++-- include/linux/filelock.h | 12 ++++++------ include/linux/fs.h | 4 ++-- 8 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index ac9034fce409..ad5b2cfe8320 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1069,7 +1069,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence) }
static int -cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **priv) +cifs_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { /* * Note that this is called by vfs setlease with i_lock held to diff --git a/fs/libfs.c b/fs/libfs.c index 4eda519c3002..1c451e76560c 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1274,7 +1274,7 @@ EXPORT_SYMBOL(alloc_anon_inode); * All arguments are ignored and it just returns -EINVAL. */ int -simple_nosetlease(struct file *filp, long arg, struct file_lock **flp, +simple_nosetlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { return -EINVAL; diff --git a/fs/locks.c b/fs/locks.c index df8b26a42524..265b5190db3e 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -438,7 +438,7 @@ static void flock_make_lock(struct file *filp, struct file_lock *fl, int type) fl->fl_end = OFFSET_MAX; }
-static int assign_type(struct file_lock *fl, long type) +static int assign_type(struct file_lock *fl, int type) { switch (type) { case F_RDLCK: @@ -549,7 +549,7 @@ static const struct lock_manager_operations lease_manager_ops = { /* * Initialize a lease, use the default lock manager operations */ -static int lease_init(struct file *filp, long type, struct file_lock *fl) +static int lease_init(struct file *filp, int type, struct file_lock *fl) { if (assign_type(fl, type) != 0) return -EINVAL; @@ -567,7 +567,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl) }
/* Allocate a file_lock initialised to this type of lease */ -static struct file_lock *lease_alloc(struct file *filp, long type) +static struct file_lock *lease_alloc(struct file *filp, int type) { struct file_lock *fl = locks_alloc_lock(); int error = -ENOMEM; @@ -1666,7 +1666,7 @@ int fcntl_getlease(struct file *filp) * conflict with the lease we're trying to set. */ static int -check_conflicting_open(struct file *filp, const long arg, int flags) +check_conflicting_open(struct file *filp, const int arg, int flags) { struct inode *inode = file_inode(filp); int self_wcount = 0, self_rcount = 0; @@ -1701,7 +1701,7 @@ check_conflicting_open(struct file *filp, const long arg, int flags) }
static int -generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv) +generic_add_lease(struct file *filp, int arg, struct file_lock **flp, void **priv) { struct file_lock *fl, *my_fl = NULL, *lease; struct inode *inode = file_inode(filp); @@ -1859,7 +1859,7 @@ static int generic_delete_lease(struct file *filp, void *owner) * The (input) flp->fl_lmops->lm_break function is required * by break_lease(). */ -int generic_setlease(struct file *filp, long arg, struct file_lock **flp, +int generic_setlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { struct inode *inode = file_inode(filp); @@ -1906,7 +1906,7 @@ lease_notifier_chain_init(void) }
static inline void -setlease_notifier(long arg, struct file_lock *lease) +setlease_notifier(int arg, struct file_lock *lease) { if (arg != F_UNLCK) srcu_notifier_call_chain(&lease_notifier_chain, arg, lease); @@ -1942,7 +1942,7 @@ EXPORT_SYMBOL_GPL(lease_unregister_notifier); * may be NULL if the lm_setup operation doesn't require it. */ int -vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) +vfs_setlease(struct file *filp, int arg, struct file_lock **lease, void **priv) { if (lease) setlease_notifier(arg, *lease); @@ -1953,7 +1953,7 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) } EXPORT_SYMBOL_GPL(vfs_setlease);
-static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) +static int do_fcntl_add_lease(unsigned int fd, struct file *filp, int arg) { struct file_lock *fl; struct fasync_struct *new; @@ -1988,7 +1988,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) * Note that you also need to call %F_SETSIG to * receive a signal when the lease is broken. */ -int fcntl_setlease(unsigned int fd, struct file *filp, long arg) +int fcntl_setlease(unsigned int fd, struct file *filp, int arg) { if (arg == F_UNLCK) return vfs_setlease(filp, F_UNLCK, NULL, (void **)&filp); diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 4c9f8bd866ab..47c5c1f86d66 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -328,7 +328,7 @@ extern int update_open_stateid(struct nfs4_state *state, const nfs4_stateid *open_stateid, const nfs4_stateid *deleg_stateid, fmode_t fmode); -extern int nfs4_proc_setlease(struct file *file, long arg, +extern int nfs4_proc_setlease(struct file *file, int arg, struct file_lock **lease, void **priv); extern int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo); diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 2563ed8580f3..26c2d3539d75 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -438,7 +438,7 @@ void nfs42_ssc_unregister_ops(void) } #endif /* CONFIG_NFS_V4_2 */
-static int nfs4_setlease(struct file *file, long arg, struct file_lock **lease, +static int nfs4_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { return nfs4_proc_setlease(file, arg, lease, priv); diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 5607b1e2b821..ba59ad558209 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -7559,7 +7559,7 @@ static int nfs4_delete_lease(struct file *file, void **priv) return generic_setlease(file, F_UNLCK, NULL, priv); }
-static int nfs4_add_lease(struct file *file, long arg, struct file_lock **lease, +static int nfs4_add_lease(struct file *file, int arg, struct file_lock **lease, void **priv) { struct inode *inode = file_inode(file); @@ -7577,7 +7577,7 @@ static int nfs4_add_lease(struct file *file, long arg, struct file_lock **lease, return -EAGAIN; }
-int nfs4_proc_setlease(struct file *file, long arg, struct file_lock **lease, +int nfs4_proc_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { switch (arg) { diff --git a/include/linux/filelock.h b/include/linux/filelock.h index efcdd1631d9b..95e868e09e29 100644 --- a/include/linux/filelock.h +++ b/include/linux/filelock.h @@ -144,7 +144,7 @@ int fcntl_setlk64(unsigned int, struct file *, unsigned int, struct flock64 *); #endif
-int fcntl_setlease(unsigned int fd, struct file *filp, long arg); +int fcntl_setlease(unsigned int fd, struct file *filp, int arg); int fcntl_getlease(struct file *filp);
/* fs/locks.c */ @@ -167,8 +167,8 @@ bool vfs_inode_has_locks(struct inode *inode); int locks_lock_inode_wait(struct inode *inode, struct file_lock *fl); int __break_lease(struct inode *inode, unsigned int flags, unsigned int type); void lease_get_mtime(struct inode *, struct timespec64 *time); -int generic_setlease(struct file *, long, struct file_lock **, void **priv); -int vfs_setlease(struct file *, long, struct file_lock **, void **); +int generic_setlease(struct file *, int, struct file_lock **, void **priv); +int vfs_setlease(struct file *, int, struct file_lock **, void **); int lease_modify(struct file_lock *, int, struct list_head *);
struct notifier_block; @@ -213,7 +213,7 @@ static inline int fcntl_setlk64(unsigned int fd, struct file *file, return -EACCES; } #endif -static inline int fcntl_setlease(unsigned int fd, struct file *filp, long arg) +static inline int fcntl_setlease(unsigned int fd, struct file *filp, int arg) { return -EINVAL; } @@ -306,13 +306,13 @@ static inline void lease_get_mtime(struct inode *inode, return; }
-static inline int generic_setlease(struct file *filp, long arg, +static inline int generic_setlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { return -EINVAL; }
-static inline int vfs_setlease(struct file *filp, long arg, +static inline int vfs_setlease(struct file *filp, int arg, struct file_lock **lease, void **priv) { return -EINVAL; diff --git a/include/linux/fs.h b/include/linux/fs.h index 8da79822dbba..0c9367980636 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1779,7 +1779,7 @@ struct file_operations { int (*flock) (struct file *, int, struct file_lock *); ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int); ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int); - int (*setlease)(struct file *, long, struct file_lock **, void **); + int (*setlease)(struct file *, int, struct file_lock **, void **); long (*fallocate)(struct file *file, int mode, loff_t offset, loff_t len); void (*show_fdinfo)(struct seq_file *m, struct file *f); @@ -2914,7 +2914,7 @@ extern int simple_write_begin(struct file *file, struct address_space *mapping, extern const struct address_space_operations ram_aops; extern int always_delete_dentry(const struct dentry *); extern struct inode *alloc_anon_inode(struct super_block *); -extern int simple_nosetlease(struct file *, long, struct file_lock **, void **); +extern int simple_nosetlease(struct file *, int, struct file_lock **, void **); extern const struct dentry_operations simple_dentry_operations;
extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
The interface for fcntl expects the argument passed for the command F_SETPIPE_SZ to be of type int. The current code wrongly treats it as a long. In order to avoid access to undefined bits, we should explicitly cast the argument to int.
Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: Jeff Layton jlayton@kernel.org Cc: Chuck Lever chuck.lever@oracle.com Cc: Kevin Brodsky Kevin.Brodsky@arm.com Cc: Vincenzo Frascino Vincenzo.Frascino@arm.com Cc: Szabolcs Nagy Szabolcs.Nagy@arm.com Cc: "Theodore Ts'o" tytso@mit.edu Cc: David Laight David.Laight@ACULAB.com Cc: Mark Rutland Mark.Rutland@arm.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-morello@op-lists.linaro.org Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- fs/pipe.c | 6 +++--- include/linux/pipe_fs_i.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c index 42c7ff41c2db..5b718342105f 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -1231,7 +1231,7 @@ const struct file_operations pipefifo_fops = { * Currently we rely on the pipe array holding a power-of-2 number * of pages. Returns 0 on error. */ -unsigned int round_pipe_size(unsigned long size) +unsigned int round_pipe_size(unsigned int size) { if (size > (1U << 31)) return 0; @@ -1314,7 +1314,7 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots) * Allocate a new array of pipe buffers and copy the info over. Returns the * pipe size if successful, or return -ERROR on error. */ -static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) +static long pipe_set_size(struct pipe_inode_info *pipe, unsigned int arg) { unsigned long user_bufs; unsigned int nr_slots, size; @@ -1382,7 +1382,7 @@ struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice) return pipe; }
-long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg) +long pipe_fcntl(struct file *file, unsigned int cmd, unsigned int arg) { struct pipe_inode_info *pipe; long ret; diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h index d2c3f16cf6b1..033d77f0c568 100644 --- a/include/linux/pipe_fs_i.h +++ b/include/linux/pipe_fs_i.h @@ -273,10 +273,10 @@ bool pipe_is_unprivileged_user(void); #ifdef CONFIG_WATCH_QUEUE int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots); #endif -long pipe_fcntl(struct file *, unsigned int, unsigned long arg); +long pipe_fcntl(struct file *, unsigned int, unsigned int arg); struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);
int create_pipe_files(struct file **, int); -unsigned int round_pipe_size(unsigned long size); +unsigned int round_pipe_size(unsigned int size);
#endif
The interface for fcntl expects the argument passed for the command F_ADD_SEALS to be of type int. The current code wrongly treats it as a long. In order to avoid access to undefined bits, we should explicitly cast the argument to int.
This commit changes the signature of all the related and helper functions so that they treat the argument as int instead of long.
Cc: Andrew Morton akpm@linux-foundation.org Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: Jeff Layton jlayton@kernel.org Cc: Chuck Lever chuck.lever@oracle.com Cc: Kevin Brodsky Kevin.Brodsky@arm.com Cc: Vincenzo Frascino Vincenzo.Frascino@arm.com Cc: Szabolcs Nagy Szabolcs.Nagy@arm.com Cc: "Theodore Ts'o" tytso@mit.edu Cc: David Laight David.Laight@ACULAB.com Cc: Mark Rutland Mark.Rutland@arm.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Cc: linux-morello@op-lists.linaro.org Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- include/linux/memfd.h | 4 ++-- mm/memfd.c | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/include/linux/memfd.h b/include/linux/memfd.h index 4f1600413f91..e7abf6fa4c52 100644 --- a/include/linux/memfd.h +++ b/include/linux/memfd.h @@ -5,9 +5,9 @@ #include <linux/file.h>
#ifdef CONFIG_MEMFD_CREATE -extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg); +extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned int arg); #else -static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned long a) +static inline long memfd_fcntl(struct file *f, unsigned int c, unsigned int a) { return -EINVAL; } diff --git a/mm/memfd.c b/mm/memfd.c index a0a7a37e8177..69b90c31d38c 100644 --- a/mm/memfd.c +++ b/mm/memfd.c @@ -243,16 +243,12 @@ static int memfd_get_seals(struct file *file) return seals ? *seals : -EINVAL; }
-long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg) +long memfd_fcntl(struct file *file, unsigned int cmd, unsigned int arg) { long error;
switch (cmd) { case F_ADD_SEALS: - /* disallow upper 32bit */ - if (arg > UINT_MAX) - return -EINVAL; - error = memfd_add_seals(file, arg); break; case F_GET_SEALS:
The interface for fcntl expects the argument passed for the command F_DIRNOTIFY to be of type int. The current code wrongly treats it as a long. In order to avoid access to undefined bits, we should explicitly cast the argument to int.
Cc: Jan Kara jack@suse.cz Cc: Amir Goldstein amir73il@gmail.com Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Christian Brauner brauner@kernel.org Cc: Jeff Layton jlayton@kernel.org Cc: Chuck Lever chuck.lever@oracle.com Cc: Kevin Brodsky Kevin.Brodsky@arm.com Cc: Vincenzo Frascino Vincenzo.Frascino@arm.com Cc: Szabolcs Nagy Szabolcs.Nagy@arm.com Cc: "Theodore Ts'o" tytso@mit.edu Cc: David Laight David.Laight@ACULAB.com Cc: Mark Rutland Mark.Rutland@arm.com Cc: linux-fsdevel@vger.kernel.org Cc: linux-morello@op-lists.linaro.org Acked-by: Jan Kara jack@suse.cz Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- fs/notify/dnotify/dnotify.c | 4 ++-- include/linux/dnotify.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 190aa717fa32..ebdcc25df0f7 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -199,7 +199,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id) }
/* this conversion is done only at watch creation */ -static __u32 convert_arg(unsigned long arg) +static __u32 convert_arg(unsigned int arg) { __u32 new_mask = FS_EVENT_ON_CHILD;
@@ -258,7 +258,7 @@ static int attach_dn(struct dnotify_struct *dn, struct dnotify_mark *dn_mark, * up here. Allocate both a mark for fsnotify to add and a dnotify_struct to be * attached to the fsnotify_mark. */ -int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) +int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg) { struct dnotify_mark *new_dn_mark, *dn_mark; struct fsnotify_mark *new_fsn_mark, *fsn_mark; diff --git a/include/linux/dnotify.h b/include/linux/dnotify.h index b1d26f9f1c9f..9f183a679277 100644 --- a/include/linux/dnotify.h +++ b/include/linux/dnotify.h @@ -30,7 +30,7 @@ struct dnotify_struct { FS_MOVED_FROM | FS_MOVED_TO)
extern void dnotify_flush(struct file *, fl_owner_t); -extern int fcntl_dirnotify(int, struct file *, unsigned long); +extern int fcntl_dirnotify(int, struct file *, unsigned int);
#else
@@ -38,7 +38,7 @@ static inline void dnotify_flush(struct file *filp, fl_owner_t id) { }
-static inline int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) +static inline int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg) { return -EINVAL; }
On Fri, Apr 14, 2023 at 04:24:54PM +0100, Luca Vizzarro wrote:
According to the documentation of fcntl, some commands take an int as argument. In practice not all of them enforce this behaviour, as they instead accept a more permissive long and in most cases not even a range check is performed.
An issue could possibly arise from a combination of the handling of the varargs in user space and the ABI rules of the target, which may result in the top bits of an int argument being non-zero.
This issue was originally raised and detailed in the following thread: https://lore.kernel.org/linux-api/Y1%2FDS6uoWP7OSkmd@arm.com/ And was discovered during the porting of Linux to Morello [1].
This series modifies the interested commands so that they explicitly take an int argument. It also propagates this change down to helper and related functions as necessary.
This series is also available on my fork at: https://git.morello-project.org/Sevenarth/linux/-/commits/fcntl-int-handling...
Best regards, Luca Vizzarro
[1] https://git.morello-project.org/morello/kernel/linux
Luca Vizzarro (5): fcntl: Cast commands with int args explicitly fs: Pass argument to fcntl_setlease as int pipe: Pass argument of pipe_fcntl as int memfd: Pass argument of memfd_fcntl as int dnotify: Pass argument of fcntl_dirnotify as int
Applied, minus the already upstreamed memfd patch.
linux-morello@op-lists.linaro.org
-
Al Viro -
Christian Brauner -
Luca Vizzarro -
Mark Rutland