linux-kernel - Re: [WTF?] aafs_create

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: <c2604d70-4c65-eb90-2cff-dd96f724222c@canonical.com>
Date:   Tue, 24 Sep 2019 10:01:30 -0700
From:   John Johansen <john.johansen@...onical.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [WTF?] aafs_create_symlink() weirdness

On 9/23/19 7:03 PM, Al Viro wrote:

WTF indeed

> 
> static struct dentry *aafs_create_symlink(const char *name,
>                                           struct dentry *parent,
>                                           const char *target,
>                                           void *private,
>                                           const struct inode_operations *iops)
> {
>         struct dentry *dent;
>         char *link = NULL;
> 
>         if (target) {
>                 if (!link)
>                         return ERR_PTR(-ENOMEM);
>         }
> 
> Er...  That's an odd way to spell
> 	if (target)
> 		return ERR_PTR(-ENOMEM);
> (and an odd error value to use).  Especially since all callers are passing
> NULL as target.
> 
> Why is that code even there, why does that argument still exist and
> how many people have actually read that function?
> 

It looks like 1180b4c757aa failed to drop it as part of the patch, but it
certainly should have. I can't say why it happened but regardless its an ugly
mistake. Thank you very much for catching this Al.

A patch to drop it is below or feel free to cons up an alternate version.

---

commit 5dbc63d4a0aa819be8ecf21a67a352dd377b0221
Author: John Johansen <john.johansen@...onical.com>
Date:   Tue Sep 24 09:46:33 2019 -0700

    apparmor: remove useless aafs_create_symlink
    
    1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata after
    replacement") reworked how the rawdata symlink is handled but failed
    to remove aafs_create_symlink which was reduced to a useless stub .
    
    Fixes: 1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata after replacement")
    Reported-by: Al Viro <viro@...iv.linux.org.uk>
    Signed-off-by: John Johansen <john.johansen@...onical.com>

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 9c0e593e30aa..308c99ea3cf8 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -335,38 +335,6 @@ static struct dentry *aafs_create_dir(const char *name, struct dentry *parent)
 			   NULL);
 }
 
-/**
- * aafs_create_symlink - create a symlink in the apparmorfs filesystem
- * @name: name of dentry to create
- * @parent: parent directory for this dentry
- * @target: if symlink, symlink target string
- * @private: private data
- * @iops: struct of inode_operations that should be used
- *
- * If @target parameter is %NULL, then the @iops parameter needs to be
- * setup to handle .readlink and .get_link inode_operations.
- */
-static struct dentry *aafs_create_symlink(const char *name,
-					  struct dentry *parent,
-					  const char *target,
-					  void *private,
-					  const struct inode_operations *iops)
-{
-	struct dentry *dent;
-	char *link = NULL;
-
-	if (target) {
-		if (!link)
-			return ERR_PTR(-ENOMEM);
-	}
-	dent = aafs_create(name, S_IFLNK | 0444, parent, private, link, NULL,
-			   iops);
-	if (IS_ERR(dent))
-		kfree(link);
-
-	return dent;
-}
-
 /**
  * aafs_remove - removes a file or directory from the apparmorfs filesystem
  *
@@ -1757,25 +1725,25 @@ int __aafs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
 	}
 
 	if (profile->rawdata) {
-		dent = aafs_create_symlink("raw_sha1", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_sha1_iops);
+		dent = aafs_create("raw_sha1", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_sha1_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);
 		profile->dents[AAFS_PROF_RAW_HASH] = dent;
 
-		dent = aafs_create_symlink("raw_abi", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_abi_iops);
+		dent = aafs_create("raw_abi", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_abi_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);
 		profile->dents[AAFS_PROF_RAW_ABI] = dent;
 
-		dent = aafs_create_symlink("raw_data", dir, NULL,
-					   profile->label.proxy,
-					   &rawdata_link_data_iops);
+		dent = aafs_create("raw_data", S_IFLNK | 0444, dir,
+				   profile->label.proxy, NULL, NULL,
+				   &rawdata_link_data_iops);
 		if (IS_ERR(dent))
 			goto fail;
 		aa_get_proxy(profile->label.proxy);