Hi,
when a vfat filesystem is mounted without the quiet option, chown fails,
but chmod still succeeds. I think that is wrong.
Run-tested, it does what I want it to do.
===
[fs/fat/]: Refine FAT chmod checks
Prohibit mode changes in non-quiet mode that cannot be stored reliably
with the on-disk format.
Signed-off-by: Jan Engelhardt <[email protected]>
---
fs/fat/file.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 4 deletions(-)
Index: linux-2.6.22/fs/fat/file.c
===================================================================
--- linux-2.6.22.orig/fs/fat/file.c
+++ linux-2.6.22/fs/fat/file.c
@@ -155,6 +155,42 @@ out:
return err;
}
+static int check_mode(const struct msdos_sb_info *sbi, mode_t mode)
+{
+ mode_t req = mode & ~S_IFMT;
+
+ /*
+ * Of the r and x bits, all (subject to umask) must be present. Of the
+ * w bits, either all (subject to umask) or none must be present.
+ */
+
+ if (S_ISREG(mode)) {
+ req &= ~sbi->options.fs_fmask;
+
+ if ((req & (S_IRUGO | S_IXUGO)) !=
+ ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_fmask))
+ return -EPERM;
+
+ if ((req & S_IWUGO) != 0 &&
+ (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_fmask))
+ return -EPERM;
+ } else if (S_ISDIR(mode)) {
+ req &= ~sbi->options.fs_dmask;
+
+ if ((req & (S_IRUGO | S_IXUGO)) !=
+ ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_dmask))
+ return -EPERM;
+
+ if ((req & S_IWUGO) != 0 &&
+ (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_dmask))
+ return -EPERM;
+ } else {
+ return -EPERM;
+ }
+
+ return 0;
+}
+
int fat_notify_change(struct dentry *dentry, struct iattr *attr)
{
struct msdos_sb_info *sbi = MSDOS_SB(dentry->d_sb);
@@ -186,16 +222,19 @@ int fat_notify_change(struct dentry *den
if (((attr->ia_valid & ATTR_UID) &&
(attr->ia_uid != sbi->options.fs_uid)) ||
((attr->ia_valid & ATTR_GID) &&
- (attr->ia_gid != sbi->options.fs_gid)) ||
- ((attr->ia_valid & ATTR_MODE) &&
- (attr->ia_mode & ~MSDOS_VALID_MODE)))
+ (attr->ia_gid != sbi->options.fs_gid)))
error = -EPERM;
-
if (error) {
if (sbi->options.quiet)
error = 0;
goto out;
}
+
+ if (error == 0 && (attr->ia_valid & ATTR_MODE))
+ if ((error = check_mode(sbi, attr->ia_mode)) != 0 &&
+ sbi->options.quiet)
+ error = 0;
+
error = inode_setattr(inode, attr);
if (error)
goto out;
Jan Engelhardt <[email protected]> writes:
> when a vfat filesystem is mounted without the quiet option, chown fails,
> but chmod still succeeds. I think that is wrong.
Could you explain why this is wrong more?
Thanks.
> [fs/fat/]: Refine FAT chmod checks
>
> Prohibit mode changes in non-quiet mode that cannot be stored reliably
> with the on-disk format.
>
> Signed-off-by: Jan Engelhardt <[email protected]>
>
> ---
> fs/fat/file.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 43 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.22/fs/fat/file.c
> ===================================================================
> --- linux-2.6.22.orig/fs/fat/file.c
> +++ linux-2.6.22/fs/fat/file.c
> @@ -155,6 +155,42 @@ out:
> return err;
> }
>
> +static int check_mode(const struct msdos_sb_info *sbi, mode_t mode)
> +{
> + mode_t req = mode & ~S_IFMT;
> +
> + /*
> + * Of the r and x bits, all (subject to umask) must be present. Of the
> + * w bits, either all (subject to umask) or none must be present.
> + */
> +
> + if (S_ISREG(mode)) {
> + req &= ~sbi->options.fs_fmask;
> +
> + if ((req & (S_IRUGO | S_IXUGO)) !=
> + ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_fmask))
> + return -EPERM;
> +
> + if ((req & S_IWUGO) != 0 &&
> + (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_fmask))
> + return -EPERM;
> + } else if (S_ISDIR(mode)) {
> + req &= ~sbi->options.fs_dmask;
> +
> + if ((req & (S_IRUGO | S_IXUGO)) !=
> + ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_dmask))
> + return -EPERM;
> +
> + if ((req & S_IWUGO) != 0 &&
> + (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_dmask))
> + return -EPERM;
> + } else {
> + return -EPERM;
> + }
> +
> + return 0;
> +}
> +
> int fat_notify_change(struct dentry *dentry, struct iattr *attr)
> {
> struct msdos_sb_info *sbi = MSDOS_SB(dentry->d_sb);
> @@ -186,16 +222,19 @@ int fat_notify_change(struct dentry *den
> if (((attr->ia_valid & ATTR_UID) &&
> (attr->ia_uid != sbi->options.fs_uid)) ||
> ((attr->ia_valid & ATTR_GID) &&
> - (attr->ia_gid != sbi->options.fs_gid)) ||
> - ((attr->ia_valid & ATTR_MODE) &&
> - (attr->ia_mode & ~MSDOS_VALID_MODE)))
> + (attr->ia_gid != sbi->options.fs_gid)))
> error = -EPERM;
> -
> if (error) {
> if (sbi->options.quiet)
> error = 0;
> goto out;
> }
> +
> + if (error == 0 && (attr->ia_valid & ATTR_MODE))
> + if ((error = check_mode(sbi, attr->ia_mode)) != 0 &&
> + sbi->options.quiet)
> + error = 0;
This test is really here? error is always "0", and anybody doesn't
check after that.
> error = inode_setattr(inode, attr);
> if (error)
> goto out;
--
OGAWA Hirofumi <[email protected]>
Hi,
On Aug 21 2007 00:17, OGAWA Hirofumi wrote:
>Jan Engelhardt <[email protected]> writes:
>> when a vfat filesystem is mounted without the quiet option, chown fails,
>> but chmod still succeeds. I think that is wrong.
>
>Could you explain why this is wrong more?
Suppose a vfat filesystem is mounted with umask=0 and [not-quiet].
Then all files will have mode 0777. Trying to change the owner will
fail, because fat does not know about owners or groups. chmod 0770,
on the other hand, will succeed, even though fat does not know about
the permission triplet [user/group/other].
So this patch changes fat's not-quiet behavior so that only UNIX
modes are accepted that can be mapped lossless between the fat disk
format and the local system. There is only one attribute, and that is
the readonly attribute, which is mapped to the UNIX write permission
bit(s). chmod 0555 is therefore valid (taking away the +w bits <=>
setting the readonly attribute). Since chmod 0775 and chmod 0755 is
an ambiguous case as to whether to set or clear the readonly bit,
these modes are also denied.
Does that help?
have a nice day,
Jan
--
Jan Engelhardt <[email protected]> writes:
> On Aug 21 2007 00:17, OGAWA Hirofumi wrote:
>>Jan Engelhardt <[email protected]> writes:
>>> when a vfat filesystem is mounted without the quiet option, chown fails,
>>> but chmod still succeeds. I think that is wrong.
>>
>>Could you explain why this is wrong more?
>
> Suppose a vfat filesystem is mounted with umask=0 and [not-quiet].
> Then all files will have mode 0777. Trying to change the owner will
> fail, because fat does not know about owners or groups. chmod 0770,
> on the other hand, will succeed, even though fat does not know about
> the permission triplet [user/group/other].
>
> So this patch changes fat's not-quiet behavior so that only UNIX
> modes are accepted that can be mapped lossless between the fat disk
> format and the local system. There is only one attribute, and that is
> the readonly attribute, which is mapped to the UNIX write permission
> bit(s). chmod 0555 is therefore valid (taking away the +w bits <=>
> setting the readonly attribute). Since chmod 0775 and chmod 0755 is
> an ambiguous case as to whether to set or clear the readonly bit,
> these modes are also denied.
I see. Sounds sane. We would be able to see what happen in real world.
However, implementation is really right?
> @@ -186,16 +222,19 @@ int fat_notify_change(struct dentry *den
> if (((attr->ia_valid & ATTR_UID) &&
> (attr->ia_uid != sbi->options.fs_uid)) ||
> ((attr->ia_valid & ATTR_GID) &&
> - (attr->ia_gid != sbi->options.fs_gid)) ||
> - ((attr->ia_valid & ATTR_MODE) &&
> - (attr->ia_mode & ~MSDOS_VALID_MODE)))
> + (attr->ia_gid != sbi->options.fs_gid)))
> error = -EPERM;
> -
> if (error) {
> if (sbi->options.quiet)
> error = 0;
> goto out;
> }
> +
> + if (error == 0 && (attr->ia_valid & ATTR_MODE))
> + if ((error = check_mode(sbi, attr->ia_mode)) != 0 &&
> + sbi->options.quiet)
> + error = 0;
This test is really here? The error is always "0", and it seems
anybody doesn't check after that.
And please separate the check like following
error = check_mode(sbi, attr->ia_mode);
if (error && sbi->options.quiet)
error = 0;
> error = inode_setattr(inode, attr);
> if (error)
> goto out;
--
OGAWA Hirofumi <[email protected]>
On Aug 21 2007 01:44, OGAWA Hirofumi wrote:
>> +
>> + if (error == 0 && (attr->ia_valid & ATTR_MODE))
>> + if ((error = check_mode(sbi, attr->ia_mode)) != 0 &&
>> + sbi->options.quiet)
>> + error = 0;
>
>This test is really here? The error is always "0", and it seems
>anybody doesn't check after that.
Thank you for the corrections. Fix below.
===
[fs/fat/]: Refine chmod checks
Prohibit mode changes in non-quiet mode that cannot be stored reliably
with the on-disk format.
Suppose a vfat filesystem is mounted with umask=0 and [not-quiet].
Then all files will have mode 0777. Trying to change the owner will
fail, because fat does not know about owners or groups. chmod 0770,
on the other hand, will succeed, even though fat does not know about
the permission triplet [user/group/other].
So this patch changes fat's not-quiet behavior so that only UNIX
modes are accepted that can be mapped lossless between the fat disk
format and the local system. There is only one attribute, and that is
the readonly attribute, which is mapped to the UNIX write permission
bit(s). chmod 0555 is therefore valid (taking away the +w bits <=>
setting the readonly attribute). Since chmod 0775 and chmod 0755 is
an ambiguous case as to whether to set or clear the readonly bit,
these modes are also denied.
In quiet mode, chmod and chown will continue to "succeed" as they did
before, meaning that a subsequent stat() will temporarily return the
new mode as long as the inode is not reread from disk, and chown will
silently do nothing, not even return the new uid/gid in stat().
Signed-off-by: Jan Engelhardt <[email protected]>
---
fs/fat/file.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 44 insertions(+), 3 deletions(-)
Index: linux-2.6.22/fs/fat/file.c
===================================================================
--- linux-2.6.22.orig/fs/fat/file.c
+++ linux-2.6.22/fs/fat/file.c
@@ -155,6 +155,42 @@ out:
return err;
}
+static int check_mode(const struct msdos_sb_info *sbi, mode_t mode)
+{
+ mode_t req = mode & ~S_IFMT;
+
+ /*
+ * Of the r and x bits, all (subject to umask) must be present. Of the
+ * w bits, either all (subject to umask) or none must be present.
+ */
+
+ if (S_ISREG(mode)) {
+ req &= ~sbi->options.fs_fmask;
+
+ if ((req & (S_IRUGO | S_IXUGO)) !=
+ ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_fmask))
+ return -EPERM;
+
+ if ((req & S_IWUGO) != 0 &&
+ (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_fmask))
+ return -EPERM;
+ } else if (S_ISDIR(mode)) {
+ req &= ~sbi->options.fs_dmask;
+
+ if ((req & (S_IRUGO | S_IXUGO)) !=
+ ((S_IRUGO | S_IXUGO) & ~sbi->options.fs_dmask))
+ return -EPERM;
+
+ if ((req & S_IWUGO) != 0 &&
+ (req & S_IWUGO) != (S_IWUGO & ~sbi->options.fs_dmask))
+ return -EPERM;
+ } else {
+ return -EPERM;
+ }
+
+ return 0;
+}
+
int fat_notify_change(struct dentry *dentry, struct iattr *attr)
{
struct msdos_sb_info *sbi = MSDOS_SB(dentry->d_sb);
@@ -186,9 +222,7 @@ int fat_notify_change(struct dentry *den
if (((attr->ia_valid & ATTR_UID) &&
(attr->ia_uid != sbi->options.fs_uid)) ||
((attr->ia_valid & ATTR_GID) &&
- (attr->ia_gid != sbi->options.fs_gid)) ||
- ((attr->ia_valid & ATTR_MODE) &&
- (attr->ia_mode & ~MSDOS_VALID_MODE)))
+ (attr->ia_gid != sbi->options.fs_gid)))
error = -EPERM;
if (error) {
@@ -196,6 +230,13 @@ int fat_notify_change(struct dentry *den
error = 0;
goto out;
}
+
+ if (attr->ia_valid & ATTR_MODE) {
+ error = check_mode(sbi, attr->ia_mode);
+ if (error != 0 && !sbi->options.quiet)
+ goto out;
+ }
+
error = inode_setattr(inode, attr);
if (error)
goto out;