From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx102.postini.com [74.125.245.102]) by kanga.kvack.org (Postfix) with SMTP id A7EB76B004A for ; Mon, 27 Feb 2012 22:44:44 -0500 (EST) Received: by lahi5 with SMTP id i5so816830lah.25 for ; Mon, 27 Feb 2012 19:44:22 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20120227144602.07f5ec33.akpm@linux-foundation.org> References: <1329990365-23779-1-git-send-email-jarkko.sakkinen@intel.com> <20120227144602.07f5ec33.akpm@linux-foundation.org> Date: Tue, 28 Feb 2012 12:44:22 +0900 Message-ID: Subject: Re: [PATCH] tmpfs: security xattr setting on inode creation From: "Ware, Ryan R" Content-Type: multipart/alternative; boundary=14dae9d718a6e01ae904b9fe0a44 Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Hugh Dickins , Jarkko Sakkinen , James Morris , linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org --14dae9d718a6e01ae904b9fe0a44 Content-Type: text/plain; charset=ISO-8859-1 On Tue, Feb 28, 2012 at 7:46 AM, Andrew Morton wrote: > On Fri, 24 Feb 2012 19:19:22 -0800 (PST) > Hugh Dickins wrote: > > > +/* > > + * Callback for security_inode_init_security() for acquiring xattrs. > > + */ > > +static int shmem_initxattrs(struct inode *inode, > > + const struct xattr *xattr_array, > > + void *fs_info) > > +{ > > + struct shmem_inode_info *info = SHMEM_I(inode); > > + const struct xattr *xattr; > > + struct shmem_xattr *new_xattr; > > + size_t len; > > + > > + for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > + new_xattr = shmem_xattr_alloc(xattr->value, > xattr->value_len); > > + if (!new_xattr) > > + return -ENOMEM; > > + > > + len = strlen(xattr->name) + 1; > > + new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len, > > + GFP_KERNEL); > > + if (!new_xattr->name) { > > + kfree(new_xattr); > > + return -ENOMEM; > > + } > > + > > + memcpy(new_xattr->name, XATTR_SECURITY_PREFIX, > > + XATTR_SECURITY_PREFIX_LEN); > > + memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN, > > + xattr->name, len); > > + > > + spin_lock(&info->lock); > > + list_add(&new_xattr->list, &info->xattr_list); > > + spin_unlock(&info->lock); > > + } > > + > > + return 0; > > +} > > So if there's a kmalloc failure partway through the array, we leave a > partially xattrified inode in place. > > Are we sure this is OK? > I'm guessing Jarkko can clean that up a bit. It wouldn't be a good idea to leave inaccurate data structures laying around during failure cases. Ryan --14dae9d718a6e01ae904b9fe0a44 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Tue, Feb 28, 2012 at 7:46 AM, Andrew Morton <= span dir=3D"ltr"><akpm@linu= x-foundation.org> wrote:
On Fri, 24 Feb 2012 19:19:22 -0800 = (PST)
Hugh Dickins <hughd@google.com&g= t; wrote:

> +/*
> + * Callback for security_inode_init_security() for acquiring xattrs.<= br> > + */
> +static int shmem_initxattrs(struct inode *inode,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const struct xattr *= xattr_array,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 void *fs_info)
> +{
> + =A0 =A0 struct shmem_inode_info *info =3D SHMEM_I(inode);
> + =A0 =A0 const struct xattr *xattr;
> + =A0 =A0 struct shmem_xattr *new_xattr;
> + =A0 =A0 size_t len;
> +
> + =A0 =A0 for (xattr =3D xattr_array; xattr->name !=3D NULL; xattr+= +) {
> + =A0 =A0 =A0 =A0 =A0 =A0 new_xattr =3D shmem_xattr_alloc(xattr->va= lue, xattr->value_len);
> + =A0 =A0 =A0 =A0 =A0 =A0 if (!new_xattr)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 len =3D strlen(xattr->name) + 1;
> + =A0 =A0 =A0 =A0 =A0 =A0 new_xattr->name =3D kmalloc(XATTR_SECURIT= Y_PREFIX_LEN + len,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 GFP_KERNEL);
> + =A0 =A0 =A0 =A0 =A0 =A0 if (!new_xattr->name) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(new_xattr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> + =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 memcpy(new_xattr->name, XATTR_SECURITY_PR= EFIX,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0XATTR_SECURITY_PREFIX_LEN); > + =A0 =A0 =A0 =A0 =A0 =A0 memcpy(new_xattr->name + XATTR_SECURITY_P= REFIX_LEN,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0xattr->name, len);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 spin_lock(&info->lock);
> + =A0 =A0 =A0 =A0 =A0 =A0 list_add(&new_xattr->list, &info-= >xattr_list);
> + =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&info->lock);
> + =A0 =A0 }
> +
> + =A0 =A0 return 0;
> +}

So if there's a kmalloc failure partway through the array, = we leave a
partially xattrified inode in place.

Are we sure this is OK?

I'm guessin= g Jarkko can clean that up a bit. =A0It wouldn't be a good idea to leav= e inaccurate data structures laying around during failure cases.

Ryan=A0
--14dae9d718a6e01ae904b9fe0a44-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org