linux-kernel - Re: [PATCH v4 1/3] xattr: add simple initxattrs function

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CA+RmS-9Pu0yK17Liwz0fqnDH-_1ejvLXRgD3y9WnZG56eoMGxA@mail.gmail.com>
Date:   Mon, 9 Jan 2017 09:41:03 -0600
From:   David Graziano <david.graziano@...kwellcollins.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     linux-security-module@...r.kernel.org,
        Paul Moore <paul@...l-moore.com>, agruenba@...hat.com,
        linux-mm@...ck.org, Stephen Smalley <sds@...ho.nsa.gov>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] xattr: add simple initxattrs function

On Sun, Jan 8, 2017 at 3:55 AM, Christoph Hellwig <hch@...radead.org> wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +int simple_xattr_initxattrs(struct inode *inode,
>> +                         const struct xattr *xattr_array,
>> +                         void *fs_info)
>> +{
>> +     struct simple_xattrs *xattrs;
>> +     const struct xattr *xattr;
>> +     struct simple_xattr *new_xattr;
>> +     size_t len;
>> +
>> +     if (!fs_info)
>> +             return -ENOMEM;
>
> This probablt should be an EINVAL, and also a WARN_ON_ONCE.

I will change the return value to -EINVAL and add the WARN_ON_ONCE.
In the next version of the patchset.

>
>> +     xattrs = (struct simple_xattrs *) fs_info;
>
> No need for the cast.  In fact we should probably just declarate it
> as struct simple_xattrs *xattrs in the protoype and thus be type safe.

I don't think the prototype can be changed to "struct simple_xattrs" as the
security_inode_init_security() function in security/security.c which calls
this is asumming an initxattrs function with following prototype
int (*initxattrs)  (struct inode *inode, const struct xattr
*xattr_array, void *fs_data)

>
>> +
>> +     for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> +             new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> +             if (!new_xattr)
>> +                     return -ENOMEM;
>
> We'll need to unwind the previous allocations here.

This patchset essentially relocates the shmem_initxattrs() function from
mm/shmem.c and uses the relocated function for both tmpfs and mqueuefs.
That inital function didn't attempt to unwind the previous allocations. If the
consensus is to unwind any allocations made by this function I can look
at adding it.