All of lore.kernel.org
 help / color / mirror / Atom feed
* fs/attr.c patch
@ 2021-09-10 12:14 Павел Самсонов
  2021-09-10 12:29 ` Carlos Maiolino
  0 siblings, 1 reply; 4+ messages in thread
From: Павел Самсонов @ 2021-09-10 12:14 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: viro

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-Signed-off-by-Pavel-Samsonov-pvsamsonov76-gmail.com.patch --]
[-- Type: text/x-patch, Size: 4623 bytes --]

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.
---
 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] 4+ messages in thread

* Re: fs/attr.c patch
  2021-09-10 12:14 fs/attr.c patch Павел Самсонов
@ 2021-09-10 12:29 ` Carlos Maiolino
  0 siblings, 0 replies; 4+ messages in thread
From: Carlos Maiolino @ 2021-09-10 12:29 UTC (permalink / raw)
  To: Павел
	Самсонов
  Cc: linux-fsdevel

On Fri, Sep 10, 2021 at 03:14:44PM +0300, Павел Самсонов wrote:
> 

Please, follow the submitting patches guide included in the documentation:

Documentation/process/submitting-patches.rst

Also, please be sure to describe why {are you changing/do you want to change} the argument from inode to dentry.


-- 
Carlos


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: fs/attr.c patch
  2011-05-19 11:57 samsonov
@ 2011-05-19 14:50 ` Serge E. Hallyn
  0 siblings, 0 replies; 4+ messages in thread
From: Serge E. Hallyn @ 2011-05-19 14:50 UTC (permalink / raw)
  To: samsonov; +Cc: linux-kernel

But you're removing the ability for a process with CAP_CHOWN to
chgrp all files.

You also change behavior (which may be wrong) when doing chgrp to
which would cause no change but the group is not in_group_p(),
which might break current users.

Quoting samsonov@krista.ru (samsonov@krista.ru):
> --- ./linux-2.6.33.4.orig/fs/attr.c	2010-05-13 02:04:27.000000000 +0400
> +++ ./linux-2.6.33.4/fs/attr.c	2011-05-19 15:43:14.533672804 +0400
> @@ -35,9 +35,7 @@
>  
>  	/* Make sure caller can chgrp. */
>  	if ((ia_valid & ATTR_GID) &&
> -	    (current_fsuid() != inode->i_uid ||
> -	    (!in_group_p(attr->ia_gid) && attr->ia_gid != inode->i_gid)) &&
> -	    !capable(CAP_CHOWN))
> +	    !(in_group_p(attr->ia_gid) && is_owner_or_cap(inode)))
>  		goto error;
>  
>  	/* Make sure a caller can chmod. */
> 
> I think that fileowner can change group of file.
> 
> 
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* fs/attr.c patch
@ 2011-05-19 11:57 samsonov
  2011-05-19 14:50 ` Serge E. Hallyn
  0 siblings, 1 reply; 4+ messages in thread
From: samsonov @ 2011-05-19 11:57 UTC (permalink / raw)
  To: linux-kernel

--- ./linux-2.6.33.4.orig/fs/attr.c	2010-05-13 02:04:27.000000000 +0400
+++ ./linux-2.6.33.4/fs/attr.c	2011-05-19 15:43:14.533672804 +0400
@@ -35,9 +35,7 @@
 
 	/* Make sure caller can chgrp. */
 	if ((ia_valid & ATTR_GID) &&
-	    (current_fsuid() != inode->i_uid ||
-	    (!in_group_p(attr->ia_gid) && attr->ia_gid != inode->i_gid)) &&
-	    !capable(CAP_CHOWN))
+	    !(in_group_p(attr->ia_gid) && is_owner_or_cap(inode)))
 		goto error;
 
 	/* Make sure a caller can chmod. */

I think that fileowner can change group of file.


 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-10 12:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 12:14 fs/attr.c patch Павел Самсонов
2021-09-10 12:29 ` Carlos Maiolino
  -- strict thread matches above, loose matches on Subject: below --
2011-05-19 11:57 samsonov
2011-05-19 14:50 ` Serge E. Hallyn

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.