linux-kernel - Re: [PATCH] eCryptfs: Privileged kthread for lower file opens

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080521151430.GG32643@halcrowt61p.austin.ibm.com>
Date:	Wed, 21 May 2008 10:14:30 -0500
From:	Michael Halcrow <mhalcrow@...ibm.com>
To:	Andrea Righi <righiandr@...rs.sourceforge.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Rusty Russell <prussell@....ibm.com>,
	"David J. Kleikamp" <shaggy@...ibm.com>,
	"SERGE E. HALLYN" <sergeh@...ibm.com>
Subject: Re: [PATCH] eCryptfs: Privileged kthread for lower file opens

On Wed, May 21, 2008 at 01:16:15AM +0200, Andrea Righi wrote:
> Michael Halcrow wrote:
>> +	req = kmem_cache_alloc(ecryptfs_open_req_cache, GFP_KERNEL);
>> +	if (!req) {
>> +		rc = -ENOMEM;
>> +		goto out;
>> +	}
>> +	mutex_init(&req->mux);
>> +	req->lower_file = lower_file;
>> +	req->lower_dentry = lower_dentry;
>> +	req->lower_mnt = lower_mnt;
>> +	req->requesting_task = current;
>> +	req->flags = 0;
>> +	mutex_lock(&ecryptfs_kthread_ctl.mux);
>> +	mutex_lock(&req->mux);
>> +	if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
>> +		rc = -EIO;
>
> Isn't a mutex_unlock(&req->mux) missing here?

Nobody else actually has a reference to req at this point, so the
unlock is not really necessary. It might not be a bad idea to jump to
out_unlock anyway just to avoid future bugs.

>> +		mutex_unlock(&ecryptfs_kthread_ctl.mux);
>> +		printk(KERN_ERR "%s: We are in the middle of shutting down; "
>> +		       "aborting privileged request to open lower file\n",
>> +			__func__);
>> +		goto out_free;
>> +	}
>> +	list_add_tail(&req->kthread_ctl_list, &ecryptfs_kthread_ctl.req_list);
>> +	mutex_unlock(&req->mux);
>> +	mutex_unlock(&ecryptfs_kthread_ctl.mux);
>> +	wake_up(&ecryptfs_kthread_ctl.wait);
>> +schedule:
>> +	set_current_state(TASK_INTERRUPTIBLE);
>> +	schedule();
>> +	mutex_lock(&req->mux);
>> +	if (req->flags == 0) {
>> +		mutex_unlock(&req->mux);
>> +		goto schedule;
>> +	}
>> +	set_current_state(TASK_RUNNING);
>> +	if (req->flags & ECRYPTFS_REQ_DROPPED
>> +	    || req->flags & ECRYPTFS_REQ_ZOMBIE) {
>> +		rc = -EIO;
>> +		printk(KERN_WARNING "%s: Privileged open request dropped\n",
>> +		       __func__);
>> +		goto out_unlock;
>> +	}
>> +	if (IS_ERR(*req->lower_file)) {
>> +		rc = PTR_ERR(*req->lower_file);
>> +		dget(lower_dentry);
>> +		mntget(lower_mnt);
>> +		(*lower_file) = dentry_open(lower_dentry, lower_mnt,
>> +					    (O_RDONLY | O_LARGEFILE));
>> +		if (IS_ERR(*lower_file)) {
>> +			rc = PTR_ERR(*req->lower_file);
>> +			(*lower_file) = NULL;
>> +			printk(KERN_WARNING "%s: Error attempting privileged "
>> +			       "open of lower file with either RW or RO "
>> +			       "perms; rc = [%d]. Giving up.\n",
>> +			       __func__, rc);
>> +		}
>> +	}
>> +out_unlock:
>> +	mutex_unlock(&req->mux);
>> +out_free:
>> +	kmem_cache_free(ecryptfs_open_req_cache, req);
>> +out:
>> +	return rc;
>> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/