lkml.org 
[lkml]   [2007]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] [PATCH -mm] file caps: make on-disk capabilities future-proof
    Quoting Stephen Smalley (sds@tycho.nsa.gov):
    > On Mon, 2007-02-19 at 11:01 -0600, Serge E. Hallyn wrote:
    > > From: Serge E. Hallyn <serue@us.ibm.com>
    > > Subject: [PATCH -mm] file caps: make on-disk capabilities future-proof
    > >
    > > Stephen Smalley has pointed out that the current file capabilities
    > > will eventually pose a problem.
    > >
    > > As the capability set changes and distributions start tagging
    > > binaries with capabilities, we would like for running an older
    > > kernel to not necessarily make those binaries unusable. To
    > > that end,
    > >
    > > 1. If capabilities are specified which we don't know
    > > about, just ignore them, do not return -EPERM as we
    > > were doing before.
    >
    > I didn't advocate that change - it is a separate issue from allowing the
    > capability bitmaps to grow in size in a backward compatible manner. In
    > the one case, you have a binary that needs a capability that is unknown
    > to the kernel, so running it could lead to unexpected failure. In the
    > other case, you simply have a binary labeled by newer userspace with a
    > newer on-disk representation that supports larger bitmaps, but the
    > binary might only have capabilities set that are known to the kernel.

    So do you think we should fail with -EINVAL in the first case?

    > > 2. Specify a size with the on-disk capability implementation.
    > > In this implementation the size is the number of __u32's
    > > used for each of (eff,perm,inh). For now, sz is 1.
    > > When we move to 64-bit capabilities, it becomes 2.
    >
    > You could alternatively split them into separate xattrs, e.g.
    > security.cap.eff, security.cap.perm, security.cap.inh, and determine the
    > bitmap size from the xattr length rather than a separate field.

    Clean, but slower... Not sure which way to go on that

    > > diff --git a/security/commoncap.c b/security/commoncap.c
    > > index be86acb..dc8bf4f 100644
    > > --- a/security/commoncap.c
    > > +++ b/security/commoncap.c
    >
    > > @@ -148,50 +145,65 @@ static int set_file_caps(struct linux_bi
    > > {
    > > struct dentry *dentry;
    > > ssize_t rc;
    > > - struct vfs_cap_data_disk dcaps;
    > > + struct vfs_cap_data_disk *dcaps;
    > > struct vfs_cap_data caps;
    > > struct inode *inode;
    > > - int err;
    > >
    > > if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)
    > > return 0;
    > >
    > > dentry = dget(bprm->file->f_dentry);
    > > inode = dentry->d_inode;
    > > - if (!inode->i_op || !inode->i_op->getxattr) {
    > > - dput(dentry);
    > > - return 0;
    > > + rc = 0;
    > > + if (!inode->i_op || !inode->i_op->getxattr)
    > > + goto out;
    > > +
    > > + rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
    > > + if (rc == -ENODATA) {
    > > + rc = 0;
    > > + goto out;
    > > + }
    >
    > I'd allocate an initial buffer with an expected size and try first to
    > avoid always having to make the two ->getxattr calls in the common case.

    I started to do that but decided that's just muck up the rfc. Will put
    it into a final version.

    > > + if (rc < 0)
    > > + goto out;
    > > + if (rc < sizeof(struct vfs_cap_data_disk)) {
    >
    > You could make this a bit stricter, as you know that it will have at
    > least three additional u32 values beyond the header.

    true.

    > > + rc = -EINVAL;
    > > + goto out;
    > > + }
    > > +
    > > + dcaps = kmalloc(rc, GFP_KERNEL);
    > > + if (!dcaps) {
    > > + rc = -ENOMEM;
    > > + goto out;
    > > + }
    > > + rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
    > > + XATTR_CAPS_SZ);
    >
    > I'm confused - you just asked for the actual length of the xattr and
    > allocated a buffer for it, and then don't use the length in this second

    Huh, I *did* send in rc, not sure what happened to that. git mis-usage
    maybe.

    > call to ->getxattr. And since you said you were organizing it as
    > eff[0..sz-1],perm[0..sz-1],inh[0..sz-1], you do need to read the entire
    > thing to get all three values even if you only use the lower 32 bits of
    > each. Or if you change the organization to avoid the need to read the
    > entire thing, you don't need the first getxattr call at all, and you

    Yes I had first organized it as eff[0],perm[0],inh[0], eff[1], etc...
    But after I changed that I did put rc back in for the length... I
    thought.

    > need to change how cap_from_disk extracts the values.

    thanks Stephen,
    -serge
    -
    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/

    \
     
     \ /
      Last update: 2007-02-20 18:53    [W:0.030 / U:0.420 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site