linux-kernel - [RFC PATCH] CIFS posix acl permission checking

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

Date:	Thu, 4 Mar 2010 11:50:04 +0100
From:	Jon Severinsson <jon@...erinsson.net>
To:	linux-cifs-client@...ts.samba.org
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [RFC PATCH] CIFS posix acl permission checking

Hello

Early this weak I sent a patch implementing posix acl permission checking in 
the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev 
as I was unaware of the linux-cifs-client list. I later tried to submit it to 
linux-cifs-client as well, but my message seems to have been lost in the 
moderation queue, so I subscribed and am trying again.

I don't believe my patch is perfect, but I think it's a good start, and would 
like some comments from more experienced cifs developers to be able to get it 
into shape for inclusion in the kernel. 

I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately 
he never followed up on my response, so I'm including some unresolved 
questions I still have, as well as attaching the patch for further comments.

Best Regards
Jon Severinsson

On Monday 01 March 2010 19:33:41, Jon Severinsson wrote:
> On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote:
>> You've included ifdefs around the check_acl entry in inode_operations,
>> *and* inside the definition of cifs_check_acl.  You only need to do
>> one or the other, and opinion is divided on which is better.
> 
> While I did recognize the redundancy, I decided to follow the same
> convention the other functions in xattr.c did, and include ifdefs at both
> locations.
> 
> I also considered the possible reasons for the existing functions to do
> both, and and came up with two reasons. The first simply being the paradigm
> of defensive programming, always check before doing a call, but never assume
> that the check has been done before being called. The second one is that of
> performance. The ifdefs has to be in cifs_check_acl to protect against other
> callers (while this patch doesn't introduce any, it doesn't prevent further
> patches from adding them), and not including the ifdefs in inode_operations
> would mean a completely useless function call when a feature was turned off
> at compile time. The second one is a micro-optimization I don't really care
> fore, but defensive programming I do respect.
> 
> With this in mind, what do you recommend, double protection, breaking
> convention or changing the existing code?
>
>>> +int cifs_check_acl(struct inode *inode, int mask)
>>> +{
>>> +	int rc = -EOPNOTSUPP;
>>> +#ifdef CONFIG_CIFS_XATTR
>>> +#ifdef CONFIG_CIFS_POSIX
>>> +	struct dentry *dentry;
>>> +	size_t buf_size;
>>> +	void *ea_value = NULL;
>>> +	ssize_t ea_size;
>>> +	struct posix_acl *acl = NULL;
>>
>> I don't think you need to initialise ea_value, do you?
> 
> While currently correct, I find it a good idea to immediately null any
> pointer that is freed in the exit section. Otherwise it is very easy to
> forget to do that the day someone adds a goto prior to the first
> assignment, and not nulling then can have unintended consequences in
> unrelated code. That being said, if you say that the kernel community
> frowns upon that kind of defensive programming I will definitely remove
> it.


View attachment "cifs-acl-permission-check-v2.patch" of type "text/x-patch" (4883 bytes)