[linux-morello] Re: [PATCH v3 1/4] fs/fcntl: Cast commands with int args explicitly

On 02/02/2023 17:06, Luca Vizzarro wrote:

According to the fcntl API specification some commands take an argument of type int and not long.

To be slightly more specific, commands that expect an integer (not a pointer) always take an int, not long.

In order to avoid access to undefined bits, we should explicitly cast the argument to int.

This commit creates an explicit cast to int of arg through a new variable called argi, and updates the relevant commands to use it.

Being slightly pickier than usual with the commit message as it's eventually for upstream: I think we can do without this paragraph, in general there isn't much point in paraphrasing what the patch does.

Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com

fs/fcntl.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c index 918d0136d12b..520520fa875b 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -316,28 +316,29 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t 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);
  

While we're at it, we could also change the prototype of setfl() accordingly.

break;

#if BITS_PER_LONG != 32 /* 32-bit arches must use fcntl64() */ @@ -374,7 +375,7 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t arg, force_successful_syscall_return(); break; case F_SETOWN:

• err = f_setown(filp, arg, 1);
  

• err = f_setown(filp, argi, 1);
  

Same here, in fact we could also simplify f_setown() a bit with s/unsigned long arg/int who/ in the prototype and removing the local variable "who".

break;

case F_GETOWN_EX: err = f_getown_ex(filp, arg); @@ -390,28 +391,28 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t 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);
  

Since you're going for a thorough clean, we could also propagate the type change to fcntl_dirnotify() and convert_arg() that it calls.

Kevin

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: