| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87pqfmgtph.fsf@linux.vnet.ibm.com>
Date: Sun, 18 Dec 2011 14:08:50 +0530
From: "Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To: Al Viro <viro@...IV.linux.org.uk>,
Sasha Levin <levinsasha928@...il.com>
Cc: ericvh@...il.com, rminnich@...dia.gov, lucho@...kov.net,
davem@...emloft.net, jvrao@...ux.vnet.ibm.com,
v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] 9p: Don't use ATTR_* values from fs.h in userspace facing structs
On Sat, 17 Dec 2011 17:47:25 +0000, Al Viro <viro@...IV.linux.org.uk> wrote:
> On Sat, Dec 17, 2011 at 05:07:02PM +0200, Sasha Levin wrote:
> > struct p9_iattr_dotl is userspace facing, but the 'valid' field is documented
> > as follows:
> >
> > * @valid: bitfield specifying which fields are valid
> > * same as in struct iattr
> >
> > Which means that the user has to know about kernel internal ATTR_* values.
> >
> > On Fri, 2011-12-16 at 23:30 +0000, Al Viro wrote:
> > > They *are* kernel internal values and 9P is asking for trouble exposing
> > > them. Translation: tomorrow we might reassign those as we bloody wish
> > > and any userland code that happens to rely on their values will break.
> > > At which point we'll handle complaints by pointing and laughing.
> > >
> > > It's a 9P bug; fix it there. Turning random internal constants into a part
> > > of ABI is not going to work.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@...il.com>
> > ---
> > fs/9p/vfs_inode_dotl.c | 31 ++++++++++++++++++++++++++++++-
> > include/net/9p/9p.h | 18 ++++++++++++++++++
> > 2 files changed, 48 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> > index 0b5745e..a948214 100644
> > --- a/fs/9p/vfs_inode_dotl.c
> > +++ b/fs/9p/vfs_inode_dotl.c
> > @@ -523,6 +523,35 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct dentry *dentry,
> > return 0;
> > }
> >
> > +int v9fs_vfs_iattr_to_9p_valid(u32 ia_valid)
> > +{
> > + u32 valid = 0, i;
> > + static u32 attr_map[][2] = {
> > + {ATTR_MODE, P9_ATTR_MODE},
> > + {ATTR_UID, P9_ATTR_UID},
> > + {ATTR_SIZE, P9_ATTR_SIZE},
> > + {ATTR_ATIME, P9_ATTR_ATIME},
> > + {ATTR_MTIME, P9_ATTR_MTIME},
> > + {ATTR_CTIME, P9_ATTR_CTIME},
> > + {ATTR_ATIME_SET, P9_ATTR_ATIME_SET},
> > + {ATTR_MTIME_SET, P9_ATTR_MTIME_SET},
> > + {ATTR_FORCE, P9_ATTR_FORCE},
> > + {ATTR_ATTR_FLAG, P9_ATTR_ATTR_FLAG},
> > + {ATTR_KILL_SUID, P9_ATTR_KILL_SUID},
> > + {ATTR_KILL_SGID, P9_ATTR_KILL_SGID},
> > + {ATTR_FILE, P9_ATTR_FILE},
> > + {ATTR_KILL_PRIV, P9_ATTR_KILL_PRIV},
> > + {ATTR_OPEN, P9_ATTR_OPEN},
> > + {ATTR_TIMES_SET, P9_ATTR_TIMES_SET},
> > + };
>
> a) ATTR_GID is lost
> b) passing ATTR_FILE is bloody pointless; look at what it does and
> realize that 9p doesn't as much as look at ia_file.
> c) ATTR_KILL_PRIV is very dubious; what's the legitimate use of that
> puppy in fs code?
>
> Look, that's the problem with exposing this stuff to protocol; you don't
> get clear semantics and are you seriously asking for trouble on kernel
> changes. Suppose tomorrow we get rid of e.g. ATTR_KILL_PRIV; what are you
> guys going to do? Hope that no 9p server has behaviour dependent on that
> flag being set or cleared?
>
> Don't turn the kernel internals into a part of ABI. And blind bulk remapping
> of constants is exactly that...
>
I am testing this locally.
commit 8ebbbcf5185096acccc523fb040e770d7876d4cd
Author: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
Date: Sat Dec 17 21:01:08 2011 +0530
fs/9p: iattr_valid flags are kernel internal flags map them to 9p values.
Kernel internal values can change, add protocol values for these constant and
use them.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 0b5745e..a81808a 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -523,6 +523,46 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct dentry *dentry,
return 0;
}
+/*
+ * Attribute flags.
+ */
+#define P9_ATTR_MODE (1 << 0)
+#define P9_ATTR_UID (1 << 1)
+#define P9_ATTR_GID (1 << 2)
+#define P9_ATTR_SIZE (1 << 3)
+#define P9_ATTR_ATIME (1 << 4)
+#define P9_ATTR_MTIME (1 << 5)
+#define P9_ATTR_CTIME (1 << 6)
+#define P9_ATTR_ATIME_SET (1 << 7)
+#define P9_ATTR_MTIME_SET (1 << 8)
+
+struct dotl_iattr_map {
+ int iattr_valid;
+ int p9_iattr_valid;
+};
+
+static int v9fs_mapped_iattr_valid(int iattr_valid)
+{
+ int i;
+ int p9_iattr_valid = 0;
+ struct dotl_iattr_map dotl_iattr_map[] = {
+ { ATTR_MODE, P9_ATTR_MODE },
+ { ATTR_UID, P9_ATTR_UID },
+ { ATTR_GID, P9_ATTR_GID },
+ { ATTR_SIZE, P9_ATTR_SIZE },
+ { ATTR_ATIME, P9_ATTR_ATIME },
+ { ATTR_MTIME, P9_ATTR_MTIME },
+ { ATTR_CTIME, P9_ATTR_CTIME },
+ { ATTR_ATIME_SET, P9_ATTR_ATIME_SET },
+ { ATTR_MTIME_SET, P9_ATTR_MTIME_SET },
+ };
+ for (i = 0; i < ARRAY_SIZE(dotl_iattr_map); i++) {
+ if (iattr_valid & dotl_iattr_map[i].iattr_valid)
+ p9_iattr_valid |= dotl_iattr_map[i].p9_iattr_valid;
+ }
+ return p9_iattr_valid;
+}
+
/**
* v9fs_vfs_setattr_dotl - set file metadata
* @dentry: file whose metadata to set
@@ -543,7 +583,7 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
if (retval)
return retval;
- p9attr.valid = iattr->ia_valid;
+ p9attr.valid = v9fs_mapped_iattr_valid(iattr->ia_valid);
p9attr.mode = iattr->ia_mode;
p9attr.uid = iattr->ia_uid;
p9attr.gid = iattr->ia_gid;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists