lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 7 Oct 2014 20:11:42 +0300
From:	Dmitry Kasatkin <dmitry.kasatkin@...il.com>
To:	Jeff Layton <jlayton@...marydata.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org,
	Christoph Hellwig <hch@...radead.org>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-security-module <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH v2 02/17] security: make security_file_set_fowner,
 f_setown and __f_setown void return

On 4 September 2014 15:38, Jeff Layton <jlayton@...marydata.com> wrote:
> security_file_set_fowner always returns 0, so make it f_setown and
> __f_setown void return functions and fix up the error handling in the
> callers.
>
> Cc: linux-security-module@...r.kernel.org
> Cc: Christoph Hellwig <hch@...radead.org>
> Signed-off-by: Jeff Layton <jlayton@...marydata.com>
> ---
>  drivers/net/tun.c           |  4 +---
>  drivers/tty/tty_io.c        |  3 ++-
>  fs/fcntl.c                  | 21 +++++++--------------
>  fs/locks.c                  |  2 +-
>  fs/notify/dnotify/dnotify.c |  8 +-------
>  include/linux/fs.h          |  4 ++--
>  include/linux/security.h    |  8 ++++----
>  net/socket.c                |  3 ++-
>  security/capability.c       |  4 ++--
>  security/security.c         |  2 +-
>  security/selinux/hooks.c    |  4 +---
>  security/smack/smack_lsm.c  |  3 +--
>  12 files changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index acaaf6784179..186ce541c657 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2152,9 +2152,7 @@ static int tun_chr_fasync(int fd, struct file *file, int on)
>                 goto out;
>
>         if (on) {
> -               ret = __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
> -               if (ret)
> -                       goto out;
> +               __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
>                 tfile->flags |= TUN_FASYNC;
>         } else
>                 tfile->flags &= ~TUN_FASYNC;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 8fbad3410c75..aea3b66f7bf2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2163,8 +2163,9 @@ static int __tty_fasync(int fd, struct file *filp, int on)
>                 }
>                 get_pid(pid);
>                 spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> -               retval = __f_setown(filp, pid, type, 0);
> +               __f_setown(filp, pid, type, 0);
>                 put_pid(pid);
> +               retval = 0;
>         }
>  out:
>         return retval;
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22d1c3df61ac..99d440a4a6ba 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -98,26 +98,19 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
>         write_unlock_irq(&filp->f_owner.lock);
>  }
>
> -int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
>                 int force)
>  {
> -       int err;
> -
> -       err = security_file_set_fowner(filp);
> -       if (err)
> -               return err;
> -
> +       security_file_set_fowner(filp);
>         f_modown(filp, pid, type, force);
> -       return 0;
>  }
>  EXPORT_SYMBOL(__f_setown);
>
> -int f_setown(struct file *filp, unsigned long arg, int force)
> +void f_setown(struct file *filp, unsigned long arg, int force)
>  {
>         enum pid_type type;
>         struct pid *pid;
>         int who = arg;
> -       int result;
>         type = PIDTYPE_PID;
>         if (who < 0) {
>                 type = PIDTYPE_PGID;
> @@ -125,9 +118,8 @@ int f_setown(struct file *filp, unsigned long arg, int force)
>         }
>         rcu_read_lock();
>         pid = find_vpid(who);
> -       result = __f_setown(filp, pid, type, force);
> +       __f_setown(filp, pid, type, force);
>         rcu_read_unlock();
> -       return result;
>  }
>  EXPORT_SYMBOL(f_setown);
>
> @@ -181,7 +173,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
>         if (owner.pid && !pid)
>                 ret = -ESRCH;
>         else
> -               ret = __f_setown(filp, pid, type, 1);
> +                __f_setown(filp, pid, type, 1);
>         rcu_read_unlock();
>
>         return ret;
> @@ -302,7 +294,8 @@ 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);
> +               f_setown(filp, arg, 1);
> +               err = 0;
>                 break;
>         case F_GETOWN_EX:
>                 err = f_getown_ex(filp, arg);
> diff --git a/fs/locks.c b/fs/locks.c
> index d7e15a256f8f..18e87f11a25f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1776,7 +1776,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
>         if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
>                 new = NULL;
>
> -       error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> +       __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
>  out_unlock:
>         spin_unlock(&inode->i_lock);
>         if (fl)
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index abc8cbcfe90e..caaaf9dfe353 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -346,13 +346,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>                 goto out;
>         }
>
> -       error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> -       if (error) {
> -               /* if we added, we must shoot */
> -               if (dn_mark == new_dn_mark)
> -                       destroy = 1;
> -               goto out;
> -       }
> +       __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
>
>         error = attach_dn(dn, dn_mark, id, fd, filp, mask);
>         /* !error means that we attached the dn to the dn_mark, so don't free it */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 435e3d9ec5cf..96528f73dda4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1139,8 +1139,8 @@ extern void fasync_free(struct fasync_struct *);
>  /* can be called from interrupts */
>  extern void kill_fasync(struct fasync_struct **, int, int);
>
> -extern int __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 void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> +extern void f_setown(struct file *filp, unsigned long arg, 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);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 623f90e5f38d..b10e7af95d3b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1559,7 +1559,7 @@ struct security_operations {
>         int (*file_lock) (struct file *file, unsigned int cmd);
>         int (*file_fcntl) (struct file *file, unsigned int cmd,
>                            unsigned long arg);
> -       int (*file_set_fowner) (struct file *file);
> +       void (*file_set_fowner) (struct file *file);
>         int (*file_send_sigiotask) (struct task_struct *tsk,
>                                     struct fown_struct *fown, int sig);
>         int (*file_receive) (struct file *file);
> @@ -1834,7 +1834,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
>                            unsigned long prot);
>  int security_file_lock(struct file *file, unsigned int cmd);
>  int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> -int security_file_set_fowner(struct file *file);
> +void security_file_set_fowner(struct file *file);
>  int security_file_send_sigiotask(struct task_struct *tsk,
>                                  struct fown_struct *fown, int sig);
>  int security_file_receive(struct file *file);
> @@ -2312,9 +2312,9 @@ static inline int security_file_fcntl(struct file *file, unsigned int cmd,
>         return 0;
>  }
>
> -static inline int security_file_set_fowner(struct file *file)
> +static inline void security_file_set_fowner(struct file *file)
>  {
> -       return 0;
> +       return;
>  }
>
>  static inline int security_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8682e7..769c9671847e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1069,7 +1069,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
>                         err = -EFAULT;
>                         if (get_user(pid, (int __user *)argp))
>                                 break;
> -                       err = f_setown(sock->file, pid, 1);
> +                       f_setown(sock->file, pid, 1);
> +                       err = 0;
>                         break;
>                 case FIOGETOWN:
>                 case SIOCGPGRP:
> diff --git a/security/capability.c b/security/capability.c
> index a74fde6a7468..d68c57a62bcf 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -343,9 +343,9 @@ static int cap_file_fcntl(struct file *file, unsigned int cmd,
>         return 0;
>  }
>
> -static int cap_file_set_fowner(struct file *file)
> +static void cap_file_set_fowner(struct file *file)
>  {
> -       return 0;
> +       return;
>  }
>
>  static int cap_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/security.c b/security/security.c
> index e41b1a8d7644..fd301c177d04 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -775,7 +775,7 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>         return security_ops->file_fcntl(file, cmd, arg);
>  }
>
> -int security_file_set_fowner(struct file *file)
> +void security_file_set_fowner(struct file *file)
>  {
>         return security_ops->file_set_fowner(file);

If file_set_fowner op is now type of "void", how you can actually
return the value?
I think compiler must give error. How could you compile it?

- Dmitry


>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b0e940497e23..ada0d0bf3463 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3346,14 +3346,12 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd,
>         return err;
>  }
>
> -static int selinux_file_set_fowner(struct file *file)
> +static void selinux_file_set_fowner(struct file *file)
>  {
>         struct file_security_struct *fsec;
>
>         fsec = file->f_security;
>         fsec->fown_sid = current_sid();
> -
> -       return 0;
>  }
>
>  static int selinux_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307ce86e..69e5635d89e5 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1390,12 +1390,11 @@ static int smack_mmap_file(struct file *file,
>   * Returns 0
>   * Further research may be required on this one.
>   */
> -static int smack_file_set_fowner(struct file *file)
> +static void smack_file_set_fowner(struct file *file)
>  {
>         struct smack_known *skp = smk_of_current();
>
>         file->f_security = skp->smk_known;
> -       return 0;
>  }
>
>  /**
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists