[PATCH 0/5] udf: Improve uid/gid handling

[PATCH 0/5] udf: Improve uid/gid handling

[Jan Kara]

Hello,

this patch series improves uid and gid handling for UDF filesystems. In
particular when uid/gid mount options are used, we make sure even newly created
and chowned files still belong to the target user/group (UDF now behaves the
same way as FAT in this mode to be easy to use for removable media). Also when
the filesystem does not store uid/gid for files, we now present these files as
belonging to overflow[ug]id instead of invalid ids if uid/gid mount option are
not used so that at least sysadmin can modify the filesystem.

								Honza

[PATCH 1/5] udf: Ignore [ug]id=ignore mount options

[Jan Kara]

Currently uid=ignore and gid=ignore make no sense without uid=<number>
and gid=<number> respectively as they result in all files having invalid
uid / gid which then doesn't allow even root to modify files and thus
causes confusion. And since commit ca76d2d8031f "UDF: fix UID and GID
mount option ignorance" (from over 10 years ago) uid=<number> overrides
all uids on disk as uid=ignore does. So just silently ignore uid=ignore
mount option.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c  |  2 --
 fs/udf/super.c  | 10 ++--------
 fs/udf/udf_sb.h | 14 ++++++--------
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index c23744d5ae5c..9021c15cec17 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1402,13 +1402,11 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 	read_lock(&sbi->s_cred_lock);
 	i_uid_write(inode, le32_to_cpu(fe->uid));
 	if (!uid_valid(inode->i_uid) ||
-	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_IGNORE) ||
 	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_SET))
 		inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
 
 	i_gid_write(inode, le32_to_cpu(fe->gid));
 	if (!gid_valid(inode->i_gid) ||
-	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_IGNORE) ||
 	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_SET))
 		inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
 
diff --git a/fs/udf/super.c b/fs/udf/super.c
index f73239a9a97d..4543338d1b0f 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -349,12 +349,8 @@ static int udf_show_options(struct seq_file *seq, struct dentry *root)
 		seq_puts(seq, ",shortad");
 	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_FORGET))
 		seq_puts(seq, ",uid=forget");
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_IGNORE))
-		seq_puts(seq, ",uid=ignore");
 	if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_FORGET))
 		seq_puts(seq, ",gid=forget");
-	if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_IGNORE))
-		seq_puts(seq, ",gid=ignore");
 	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
 		seq_printf(seq, ",uid=%u", from_kuid(&init_user_ns, sbi->s_uid));
 	if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
@@ -610,14 +606,12 @@ static int udf_parse_options(char *options, struct udf_options *uopt,
 			uopt->flags |= (1 << UDF_FLAG_NLS_MAP);
 			break;
 #endif
-		case Opt_uignore:
-			uopt->flags |= (1 << UDF_FLAG_UID_IGNORE);
-			break;
 		case Opt_uforget:
 			uopt->flags |= (1 << UDF_FLAG_UID_FORGET);
 			break;
+		case Opt_uignore:
 		case Opt_gignore:
-			uopt->flags |= (1 << UDF_FLAG_GID_IGNORE);
+			/* These options are superseeded by uid=<number> */
 			break;
 		case Opt_gforget:
 			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 68c9f1d618f5..9dcb475fc74e 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -23,14 +23,12 @@
 #define UDF_FLAG_NLS_MAP		9
 #define UDF_FLAG_UTF8			10
 #define UDF_FLAG_UID_FORGET     11    /* save -1 for uid to disk */
-#define UDF_FLAG_UID_IGNORE     12    /* use sb uid instead of on disk uid */
-#define UDF_FLAG_GID_FORGET     13
-#define UDF_FLAG_GID_IGNORE     14
-#define UDF_FLAG_UID_SET	15
-#define UDF_FLAG_GID_SET	16
-#define UDF_FLAG_SESSION_SET	17
-#define UDF_FLAG_LASTBLOCK_SET	18
-#define UDF_FLAG_BLOCKSIZE_SET	19
+#define UDF_FLAG_GID_FORGET     12
+#define UDF_FLAG_UID_SET	13
+#define UDF_FLAG_GID_SET	14
+#define UDF_FLAG_SESSION_SET	15
+#define UDF_FLAG_LASTBLOCK_SET	16
+#define UDF_FLAG_BLOCKSIZE_SET	17
 
 #define UDF_PART_FLAG_UNALLOC_BITMAP	0x0001
 #define UDF_PART_FLAG_UNALLOC_TABLE	0x0002
-- 
2.13.6

[PATCH 2/5] udf: Apply uid/gid mount options also to new inodes & chown

[Jan Kara]

Currently newly created files belong to current user despite
uid=<number> / gid=<number> mount options. This is confusing to users
(as owner of the file will change after remount / eviction from cache)
and also inconsistent with e.g. FAT with the same mount option. So apply
uid=<number> and gid=<number> also to newly created inodes and similarly
as FAT disallow to change owner of the file in this case.

Reported-by: Steve Kenton <skenton@ou.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c   | 10 ++++++++++
 fs/udf/ialloc.c |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 356c2bf148a5..cd31e4f6d6da 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -257,12 +257,22 @@ const struct file_operations udf_file_operations = {
 static int udf_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = d_inode(dentry);
+	struct super_block *sb = inode->i_sb;
 	int error;
 
 	error = setattr_prepare(dentry, attr);
 	if (error)
 		return error;
 
+	if ((attr->ia_valid & ATTR_UID) &&
+	    UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET) &&
+	    !uid_eq(attr->ia_uid, UDF_SB(sb)->s_uid))
+		return -EPERM;
+	if ((attr->ia_valid & ATTR_GID) &&
+	    UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET) &&
+	    !gid_eq(attr->ia_gid, UDF_SB(sb)->s_gid))
+		return -EPERM;
+
 	if ((attr->ia_valid & ATTR_SIZE) &&
 	    attr->ia_size != i_size_read(inode)) {
 		error = udf_setsize(inode, attr->ia_size);
diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index b6e420c1bfeb..b7a0d4b4bda1 100644
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -104,6 +104,10 @@ struct inode *udf_new_inode(struct inode *dir, umode_t mode)
 	}
 
 	inode_init_owner(inode, dir, mode);
+	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
+		inode->i_uid = sbi->s_uid;
+	if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
+		inode->i_gid = sbi->s_gid;
 
 	iinfo->i_location.logicalBlockNum = block;
 	iinfo->i_location.partitionReferenceNum =
-- 
2.13.6

[PATCH 3/5] udf: Clean up handling of invalid uid/gid

[Jan Kara]

Current code relies on the fact that invalid uid/gid as defined by UDF
2.60 3.3.3.1 and 3.3.3.2 coincides with invalid uid/gid as used by the
user namespaces implementation. Since this is only lucky coincidence,
clean this up to avoid future surprises in case user namespaces
implementation changes. Also this is more robust in presence of valid
(from UDF point of view) uids / gids which do not map into current user
namespace.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c   | 21 +++++++++++++--------
 fs/udf/udfdecl.h |  2 ++
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 9021c15cec17..c80765d62f7e 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1275,6 +1275,7 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 	unsigned int indirections = 0;
 	int bs = inode->i_sb->s_blocksize;
 	int ret = -EIO;
+	uint32_t uid, gid;
 
 reread:
 	if (iloc->partitionReferenceNum >= sbi->s_partitions) {
@@ -1400,15 +1401,19 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 
 	ret = -EIO;
 	read_lock(&sbi->s_cred_lock);
-	i_uid_write(inode, le32_to_cpu(fe->uid));
-	if (!uid_valid(inode->i_uid) ||
+	uid = le32_to_cpu(fe->uid);
+	if (uid == UDF_INVALID_ID ||
 	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_SET))
-		inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
+		inode->i_uid = sbi->s_uid;
+	else
+		i_uid_write(inode, uid);
 
-	i_gid_write(inode, le32_to_cpu(fe->gid));
-	if (!gid_valid(inode->i_gid) ||
+	gid = le32_to_cpu(fe->gid);
+	if (gid == UDF_INVALID_ID ||
 	    UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_SET))
-		inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
+		inode->i_gid = sbi->s_gid;
+	else
+		i_gid_write(inode, gid);
 
 	if (fe->icbTag.fileType != ICBTAG_FILE_TYPE_DIRECTORY &&
 			sbi->s_fmode != UDF_INVALID_MODE)
@@ -1653,12 +1658,12 @@ static int udf_update_inode(struct inode *inode, int do_sync)
 	}
 
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
-		fe->uid = cpu_to_le32(-1);
+		fe->uid = cpu_to_le32(UDF_INVALID_ID);
 	else
 		fe->uid = cpu_to_le32(i_uid_read(inode));
 
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
-		fe->gid = cpu_to_le32(-1);
+		fe->gid = cpu_to_le32(UDF_INVALID_ID);
 	else
 		fe->gid = cpu_to_le32(i_gid_read(inode));
 
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index f5e0fe78979e..68e8a64d22e0 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -48,6 +48,8 @@ extern __printf(3, 4) void _udf_warn(struct super_block *sb,
 #define UDF_EXTENT_LENGTH_MASK	0x3FFFFFFF
 #define UDF_EXTENT_FLAG_MASK	0xC0000000
 
+#define UDF_INVALID_ID ((uint32_t)-1)
+
 #define UDF_NAME_PAD		4
 #define UDF_NAME_LEN		254
 #define UDF_NAME_LEN_CS0	255
-- 
2.13.6

[PATCH 4/5] udf: Provide saner default for invalid uid / gid

[Jan Kara]

Currently when UDF filesystem is recorded without uid / gid (ids are set
to -1), we will assign INVALID_[UG]ID to vfs inode unless user uses uid=
and gid= mount options. In such case filesystem could not be modified in
any way as VFS refuses to modify files with invalid ids (even by root).
This is confusing to users and not very useful default since such media
mode is generally used for removable media. Use overflow[ug]id instead
so that at least root can modify the filesystem.

Reported-by: Steve Kenton <skenton@ou.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 4543338d1b0f..79a949d4a373 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2085,8 +2085,9 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	bool lvid_open = false;
 
 	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
-	uopt.uid = INVALID_UID;
-	uopt.gid = INVALID_GID;
+	/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
+	uopt.uid = make_kuid(current_user_ns(), overflowuid);
+	uopt.gid = make_kgid(current_user_ns(), overflowgid);
 	uopt.umask = 0;
 	uopt.fmode = UDF_INVALID_MODE;
 	uopt.dmode = UDF_INVALID_MODE;
-- 
2.13.6

[PATCH 5/5] udf: Update mount option documentation

[Jan Kara]

Update documentation of uid and gid mount options.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 Documentation/filesystems/udf.txt | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/Documentation/filesystems/udf.txt b/Documentation/filesystems/udf.txt
index d3d0e3218f86..cf30bdcdcc4f 100644
--- a/Documentation/filesystems/udf.txt
+++ b/Documentation/filesystems/udf.txt
@@ -36,18 +36,14 @@ The following mount options are supported:
 	iocharset=	Set the NLS character set
 
 The uid= and gid= options need a bit more explaining.  They will accept a
-decimal numeric value which will be used as the default ID for that mount.
-They will also accept the string "ignore" and "forget".  For files on the disk
-that are owned by nobody ( -1 ), they will instead look as if they are owned
-by the default ID.  The ignore option causes the default ID to override all
-IDs on the disk, not just -1.  The forget option causes all IDs to be written
-to disk as -1, so when the media is later remounted, they will appear to be
-owned by whatever default ID it is mounted with at that time.
+decimal numeric value and all inodes on that mount will then appear as
+belonging to that uid and gid.  Mount options also accept the string "forget".
+The forget option causes all IDs to be written to disk as -1 which is a way
+of UDF standard to indicate that IDs are not supported for these files .
 
-For typical desktop use of removable media, you should set the ID to that
-of the interactively logged on user, and also specify both the forget and
-ignore options.  This way the interactive user will always see the files
-on the disk as belonging to him.
+For typical desktop use of removable media, you should set the ID to that of
+the interactively logged on user, and also specify the forget option.  This way
+the interactive user will always see the files on the disk as belonging to him.
 
 The remaining are for debugging and disaster recovery:
 
-- 
2.13.6

Re: [PATCH 0/5] udf: Improve uid/gid handling

[Pali Rohár]

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

On Thursday 22 February 2018 11:45:14 Jan Kara wrote:
> Hello,
> 
> this patch series improves uid and gid handling for UDF filesystems. In
> particular when uid/gid mount options are used, we make sure even newly created
> and chowned files still belong to the target user/group (UDF now behaves the
> same way as FAT in this mode to be easy to use for removable media). Also when
> the filesystem does not store uid/gid for files, we now present these files as
> belonging to overflow[ug]id instead of invalid ids if uid/gid mount option are
> not used so that at least sysadmin can modify the filesystem.
> 
> 								Honza

Looks good, this should improve handling of uid/gid on removable UDF
filesystems. Add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

Anyway, I would propose backporting patch "[PATCH 4/5] udf: Provide
saner default for invalid uid / gid" into stable kernels to allow root
user to modify files without uid/gid.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 0/5] udf: Improve uid/gid handling

[Jan Kara]

On Sat 24-02-18 12:56:07, Pali Roh�r wrote:
> On Thursday 22 February 2018 11:45:14 Jan Kara wrote:
> > Hello,
> > 
> > this patch series improves uid and gid handling for UDF filesystems. In
> > particular when uid/gid mount options are used, we make sure even newly created
> > and chowned files still belong to the target user/group (UDF now behaves the
> > same way as FAT in this mode to be easy to use for removable media). Also when
> > the filesystem does not store uid/gid for files, we now present these files as
> > belonging to overflow[ug]id instead of invalid ids if uid/gid mount option are
> > not used so that at least sysadmin can modify the filesystem.
> > 
> > 								Honza
> 
> Looks good, this should improve handling of uid/gid on removable UDF
> filesystems. Add my Reviewed-by: Pali Roh�r <pali.rohar@gmail.com>

Thanks for review!

> Anyway, I would propose backporting patch "[PATCH 4/5] udf: Provide
> saner default for invalid uid / gid" into stable kernels to allow root
> user to modify files without uid/gid.

Also patch 3/5 would be needed to make this change safe in presence of user
namespaces. But as much as the problem is annoying when it happens, I
didn't get many reports of it over the years (just Steve and you when you
tried to modify UDF tools) and it can be worked around by using
uid=<something> mount option so I don't think stable backport is really
warranted.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/5] udf: Improve uid/gid handling

[Pali Rohár]

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

On Monday 26 February 2018 14:40:31 Jan Kara wrote:
> On Sat 24-02-18 12:56:07, Pali Rohár wrote:
> > On Thursday 22 February 2018 11:45:14 Jan Kara wrote:
> > > Hello,
> > > 
> > > this patch series improves uid and gid handling for UDF filesystems. In
> > > particular when uid/gid mount options are used, we make sure even newly created
> > > and chowned files still belong to the target user/group (UDF now behaves the
> > > same way as FAT in this mode to be easy to use for removable media). Also when
> > > the filesystem does not store uid/gid for files, we now present these files as
> > > belonging to overflow[ug]id instead of invalid ids if uid/gid mount option are
> > > not used so that at least sysadmin can modify the filesystem.
> > > 
> > > 								Honza
> > 
> > Looks good, this should improve handling of uid/gid on removable UDF
> > filesystems. Add my Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Thanks for review!
> 
> > Anyway, I would propose backporting patch "[PATCH 4/5] udf: Provide
> > saner default for invalid uid / gid" into stable kernels to allow root
> > user to modify files without uid/gid.
> 
> Also patch 3/5 would be needed to make this change safe in presence of user
> namespaces. But as much as the problem is annoying when it happens, I
> didn't get many reports of it over the years (just Steve and you when you
> tried to modify UDF tools) and it can be worked around by using
> uid=<something> mount option so I don't think stable backport is really
> warranted.

Ok, then let it as is.

Anyway, should not be this uid= and gid= handling implemented in genetic
VFS layer instead in filesystem drivers?

Because e.g. missing options uid= and gid= in ext4 filesystem driver
make usage of ext4 on removable media really problematic. Basically any
filesystem which implement permissions is due to this reason problematic
for removable media which is supposed to be read/write by anybody.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 0/5] udf: Improve uid/gid handling

[Jan Kara]

On Mon 26-02-18 20:18:11, Pali Roh�r wrote:
> On Monday 26 February 2018 14:40:31 Jan Kara wrote:
> > On Sat 24-02-18 12:56:07, Pali Roh�r wrote:
> > > On Thursday 22 February 2018 11:45:14 Jan Kara wrote:
> > > > Hello,
> > > > 
> > > > this patch series improves uid and gid handling for UDF filesystems. In
> > > > particular when uid/gid mount options are used, we make sure even newly created
> > > > and chowned files still belong to the target user/group (UDF now behaves the
> > > > same way as FAT in this mode to be easy to use for removable media). Also when
> > > > the filesystem does not store uid/gid for files, we now present these files as
> > > > belonging to overflow[ug]id instead of invalid ids if uid/gid mount option are
> > > > not used so that at least sysadmin can modify the filesystem.
> > > > 
> > > > 								Honza
> > > 
> > > Looks good, this should improve handling of uid/gid on removable UDF
> > > filesystems. Add my Reviewed-by: Pali Roh�r <pali.rohar@gmail.com>
> > 
> > Thanks for review!
> > 
> > > Anyway, I would propose backporting patch "[PATCH 4/5] udf: Provide
> > > saner default for invalid uid / gid" into stable kernels to allow root
> > > user to modify files without uid/gid.
> > 
> > Also patch 3/5 would be needed to make this change safe in presence of user
> > namespaces. But as much as the problem is annoying when it happens, I
> > didn't get many reports of it over the years (just Steve and you when you
> > tried to modify UDF tools) and it can be worked around by using
> > uid=<something> mount option so I don't think stable backport is really
> > warranted.
> 
> Ok, then let it as is.
> 
> Anyway, should not be this uid= and gid= handling implemented in genetic
> VFS layer instead in filesystem drivers?

There was a discussion about this but it never ended up with a real code...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR