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%[email protected]/
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-v2
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(-)
--
2.34.1
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 <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Chuck Lever <[email protected]>
Cc: Kevin Brodsky <[email protected]>
Cc: Vincenzo Frascino <[email protected]>
Cc: Szabolcs Nagy <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Cc: David Laight <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luca Vizzarro <[email protected]>
---
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
--
2.34.1
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 <[email protected]>
Cc: Amir Goldstein <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Chuck Lever <[email protected]>
Cc: Kevin Brodsky <[email protected]>
Cc: Vincenzo Frascino <[email protected]>
Cc: Szabolcs Nagy <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Cc: David Laight <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Acked-by: Jan Kara <[email protected]>
Signed-off-by: Luca Vizzarro <[email protected]>
---
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;
}
--
2.34.1
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 <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Chuck Lever <[email protected]>
Cc: Kevin Brodsky <[email protected]>
Cc: Vincenzo Frascino <[email protected]>
Cc: Szabolcs Nagy <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
Cc: David Laight <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Luca Vizzarro <[email protected]>
---
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:
--
2.34.1
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%[email protected]/
> 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-v2
>
> 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.