* [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com>
@ 2021-09-10 15:57 Павел Самсонов
2021-09-13 8:14 ` Carlos Maiolino
0 siblings, 1 reply; 2+ messages in thread
From: Павел Самсонов @ 2021-09-10 15:57 UTC (permalink / raw)
To: linux-fsdevel; +Cc: viro
From 07b6f881080fa18ac404054d43b99433275fe966 Mon Sep 17 00:00:00 2001
From: Pavel Samsonov <pvsamsonov76@gmail.com>
Date: Fri, 10 Sep 2021 14:53:39 +0300
Subject: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com>
The patch changes the argument of chown_ok(), chgrp_ok() ...
functions from *inode to *dentry. The setattr_prepare() function
has an argument * dentry; it is logical to work with the dentry
structure in the condition checking functions as well.
---
fs/attr.c | 45 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index 473d21b3a86d..de1898c19bde 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -21,7 +21,7 @@
/**
* chown_ok - verify permissions to chown inode
* @mnt_userns: user namespace of the mount @inode was found from
- * @inode: inode to check permissions on
+ * @dentry: dentry to check permissions on
* @uid: uid to chown @inode to
*
* If the inode has been found through an idmapped mount the user
namespace of
@@ -31,9 +31,11 @@
* performed on the raw inode simply passs init_user_ns.
*/
static bool chown_ok(struct user_namespace *mnt_userns,
- const struct inode *inode,
+ const struct dentry *dentry,
kuid_t uid)
{
+ struct inode *inode = d_inode(dentry);
+
kuid_t kuid = i_uid_into_mnt(mnt_userns, inode);
if (uid_eq(current_fsuid(), kuid) && uid_eq(uid, kuid))
return true;
@@ -48,7 +50,7 @@ static bool chown_ok(struct user_namespace *mnt_userns,
/**
* chgrp_ok - verify permissions to chgrp inode
* @mnt_userns: user namespace of the mount @inode was found from
- * @inode: inode to check permissions on
+ * @dentry: dentry to check permissions on
* @gid: gid to chown @inode to
*
* If the inode has been found through an idmapped mount the user
namespace of
@@ -58,8 +60,10 @@ static bool chown_ok(struct user_namespace *mnt_userns,
* performed on the raw inode simply passs init_user_ns.
*/
static bool chgrp_ok(struct user_namespace *mnt_userns,
- const struct inode *inode, kgid_t gid)
+ const struct dentry *dentry, kgid_t gid)
{
+ struct inode *inode = d_inode(dentry);
+
kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode)) &&
(in_group_p(gid) || gid_eq(gid, kgid)))
@@ -72,6 +76,27 @@ static bool chgrp_ok(struct user_namespace *mnt_userns,
return false;
}
+/**
+ * fowner_ok - verify permissions to chmod inode
+ * @mnt_userns: user namespace of the mount @inode was found from
+ * @dentry: dentry to check permissions on
+ *
+ * If the inode has been found through an idmapped mount the user
namespace of
+ * the vfsmount must be passed through @mnt_userns. This function will then
+ * take care to map the inode according to @mnt_userns before checking
+ * permissions. On non-idmapped mounts or if permission checking is to be
+ * performed on the raw inode simply passs init_user_ns.
+ */
+static bool fowner_ok(struct user_namespace *mnt_userns,
+ const struct dentry *dentry)
+{
+ struct inode *inode = d_inode(dentry);
+
+ if (inode_owner_or_capable(mnt_userns, inode))
+ return true;
+ return false;
+}
+
/**
* setattr_prepare - check if attribute changes to a dentry are allowed
* @mnt_userns: user namespace of the mount the inode was found from
@@ -114,27 +139,31 @@ int setattr_prepare(struct user_namespace
*mnt_userns, struct dentry *dentry,
goto kill_priv;
/* Make sure a caller can chown. */
- if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, inode,
attr->ia_uid))
+ if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, dentry,
attr->ia_uid))
return -EPERM;
/* Make sure caller can chgrp. */
- if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, inode,
attr->ia_gid))
+ if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, dentry,
attr->ia_gid))
return -EPERM;
/* Make sure a caller can chmod. */
if (ia_valid & ATTR_MODE) {
- if (!inode_owner_or_capable(mnt_userns, inode))
+ if (!fowner_ok(mnt_userns, dentry))
return -EPERM;
/* Also check the setgid bit! */
if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
i_gid_into_mnt(mnt_userns, inode)) &&
!capable_wrt_inode_uidgid(mnt_userns, inode,
CAP_FSETID))
attr->ia_mode &= ~S_ISGID;
+ /* Also check the setuid bit! */
+ if (!(capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID) ||
+ uid_eq(current_fsuid(), inode->i_uid)))
+ attr->ia_mode &= ~S_ISUID;
}
/* Check for setting the inode time. */
if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
- if (!inode_owner_or_capable(mnt_userns, inode))
+ if (!fowner_ok(mnt_userns, dentry))
return -EPERM;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com>
2021-09-10 15:57 [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com> Павел Самсонов
@ 2021-09-13 8:14 ` Carlos Maiolino
0 siblings, 0 replies; 2+ messages in thread
From: Carlos Maiolino @ 2021-09-13 8:14 UTC (permalink / raw)
To: Павел
Самсонов
Cc: linux-fsdevel
On Fri, Sep 10, 2021 at 06:57:13PM +0300, Павел Самсонов wrote:
> From 07b6f881080fa18ac404054d43b99433275fe966 Mon Sep 17 00:00:00 2001
> From: Pavel Samsonov <pvsamsonov76@gmail.com>
> Date: Fri, 10 Sep 2021 14:53:39 +0300
> Subject: [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com>
>
> The patch changes the argument of chown_ok(), chgrp_ok() ...
> functions from *inode to *dentry. The setattr_prepare() function
> has an argument * dentry; it is logical to work with the dentry
> structure in the condition checking functions as well.
Hmmm.
I particularly don't see how this is useful in any way, both chown_ok() and
chgrp_ok() works on the inode itself, not in the dentry. Same goes for
inode_owner_or_capable().
So, you are adding a lot of boiler plate code to each function (and adding a new
extra helper function), just to extract the inode from the dentry on each
helper, instead of just pass the inode directly as how it's ok.
IMHO, just because setattr_prepare() receives a dentry, doesn't mean it needs to
pass the same dentry to the helpers, because, as I said, the helpers itself acts
on the inodes, not in the dentry, so, this patch just adds a lot of unnecessary
code.
But, well, this is my own personal opinion.
Also, for the next time/patches, the patch's subject needs to have the patch
subject, not your SoB.
> ---
> fs/attr.c | 45 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 473d21b3a86d..de1898c19bde 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -21,7 +21,7 @@
> /**
> * chown_ok - verify permissions to chown inode
> * @mnt_userns: user namespace of the mount @inode was found from
> - * @inode: inode to check permissions on
> + * @dentry: dentry to check permissions on
> * @uid: uid to chown @inode to
> *
> * If the inode has been found through an idmapped mount the user namespace
> of
> @@ -31,9 +31,11 @@
> * performed on the raw inode simply passs init_user_ns.
> */
> static bool chown_ok(struct user_namespace *mnt_userns,
> - const struct inode *inode,
> + const struct dentry *dentry,
> kuid_t uid)
> {
> + struct inode *inode = d_inode(dentry);
> +
> kuid_t kuid = i_uid_into_mnt(mnt_userns, inode);
> if (uid_eq(current_fsuid(), kuid) && uid_eq(uid, kuid))
> return true;
> @@ -48,7 +50,7 @@ static bool chown_ok(struct user_namespace *mnt_userns,
> /**
> * chgrp_ok - verify permissions to chgrp inode
> * @mnt_userns: user namespace of the mount @inode was found from
> - * @inode: inode to check permissions on
> + * @dentry: dentry to check permissions on
> * @gid: gid to chown @inode to
> *
> * If the inode has been found through an idmapped mount the user namespace
> of
> @@ -58,8 +60,10 @@ static bool chown_ok(struct user_namespace *mnt_userns,
> * performed on the raw inode simply passs init_user_ns.
> */
> static bool chgrp_ok(struct user_namespace *mnt_userns,
> - const struct inode *inode, kgid_t gid)
> + const struct dentry *dentry, kgid_t gid)
> {
> + struct inode *inode = d_inode(dentry);
> +
> kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
> if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode)) &&
> (in_group_p(gid) || gid_eq(gid, kgid)))
> @@ -72,6 +76,27 @@ static bool chgrp_ok(struct user_namespace *mnt_userns,
> return false;
> }
>
> +/**
> + * fowner_ok - verify permissions to chmod inode
> + * @mnt_userns: user namespace of the mount @inode was found from
> + * @dentry: dentry to check permissions on
> + *
> + * If the inode has been found through an idmapped mount the user namespace
> of
> + * the vfsmount must be passed through @mnt_userns. This function will then
> + * take care to map the inode according to @mnt_userns before checking
> + * permissions. On non-idmapped mounts or if permission checking is to be
> + * performed on the raw inode simply passs init_user_ns.
> + */
> +static bool fowner_ok(struct user_namespace *mnt_userns,
> + const struct dentry *dentry)
> +{
> + struct inode *inode = d_inode(dentry);
> +
> + if (inode_owner_or_capable(mnt_userns, inode))
> + return true;
> + return false;
> +}
> +
> /**
> * setattr_prepare - check if attribute changes to a dentry are allowed
> * @mnt_userns: user namespace of the mount the inode was found from
> @@ -114,27 +139,31 @@ int setattr_prepare(struct user_namespace *mnt_userns,
> struct dentry *dentry,
> goto kill_priv;
>
> /* Make sure a caller can chown. */
> - if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, inode,
> attr->ia_uid))
> + if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, dentry,
> attr->ia_uid))
> return -EPERM;
>
> /* Make sure caller can chgrp. */
> - if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, inode,
> attr->ia_gid))
> + if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, dentry,
> attr->ia_gid))
> return -EPERM;
>
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> - if (!inode_owner_or_capable(mnt_userns, inode))
> + if (!fowner_ok(mnt_userns, dentry))
> return -EPERM;
> /* Also check the setgid bit! */
> if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
> i_gid_into_mnt(mnt_userns, inode)) &&
> !capable_wrt_inode_uidgid(mnt_userns, inode,
> CAP_FSETID))
> attr->ia_mode &= ~S_ISGID;
> + /* Also check the setuid bit! */
> + if (!(capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID) ||
> + uid_eq(current_fsuid(), inode->i_uid)))
> + attr->ia_mode &= ~S_ISUID;
> }
>
> /* Check for setting the inode time. */
> if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
> - if (!inode_owner_or_capable(mnt_userns, inode))
> + if (!fowner_ok(mnt_userns, dentry))
> return -EPERM;
> }
>
> --
> 2.30.2
>
>
--
Carlos
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-13 8:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 15:57 [PATCH] Signed-off-by: Pavel Samsonov <pvsamsonov76@gmail.com> Павел Самсонов
2021-09-13 8:14 ` Carlos Maiolino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.