linux-kernel - Re: [PATCH] Fix common_audit_data type for smack_inode_unlink() and smack_inode

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]

Date:	Tue, 12 Mar 2013 13:30:00 +0400
From:	Igor Zhbanov <i.zhbanov@...sung.com>
To:	Casey Schaufler <casey@...aufler-ca.com>
Cc:	linux-kernel@...r.kernel.org,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH] Fix common_audit_data type for smack_inode_unlink() and
 smack_inode_rmdir()

Hi Casey,

Casey Schaufler wrote:

> On 3/11/2013 4:50 AM, Igor Zhbanov wrote:
>> This patch fixes kernel Oops because of wrong common_audit_data type
>> in smack_inode_unlink() and smack_inode_rmdir().
> This patch does not apply to any tree I have. The lines:
>
> 	smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
>
> that the patch is deleting are not present. Can you make
> sure that the patch applies to Linus' tree and resubmit?

My patch is applied against linux-next:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/security/smack/smack_lsm.c

And the patch successfully applies to linux-3.8.2 too.
What tree you have?

> Thank you.
>> When SMACK security module is enabled and SMACK logging is on (/smack/logging
>> is not zero) and you try to delete the file which
>> 1) you cannot delete due to SMACK rules and logging of failures is on
>> or
>> 2) you can delete and logging of success is on,
>>
>> you will see following:
>>
>> 	Unable to handle kernel NULL pointer dereference at virtual address 000002d7
>>
>> 	[<...>] (strlen+0x0/0x28)
>> 	[<...>] (audit_log_untrustedstring+0x14/0x28)
>> 	[<...>] (common_lsm_audit+0x108/0x6ac)
>> 	[<...>] (smack_log+0xc4/0xe4)
>> 	[<...>] (smk_curacc+0x80/0x10c)
>> 	[<...>] (smack_inode_unlink+0x74/0x80)
>> 	[<...>] (security_inode_unlink+0x2c/0x30)
>> 	[<...>] (vfs_unlink+0x7c/0x100)
>> 	[<...>] (do_unlinkat+0x144/0x16c)
>>
>> The function smack_inode_unlink() (and smack_inode_rmdir()) need
>> to log two structures of different types. First of all it does:
>>
>> 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
>> 	smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
>>
>> This will set common audit data type to LSM_AUDIT_DATA_DENTRY
>> and store dentry for auditing (by function smk_curacc(), which in turn calls
>> dump_common_audit_data(), which is actually uses provided data and logs it).
>>
>> 	/*
>> 	 * You need write access to the thing you're unlinking
>> 	 */
>> 	rc = smk_curacc(smk_of_inode(ip), MAY_WRITE, &ad);
>> 	if (rc == 0) {
>> 		/*
>> 		 * You also need write access to the containing directory
>> 		 */
>>
>> Then this function wants to log anoter data:
>>
>> 		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
>> 		smk_ad_setfield_u_fs_inode(&ad, dir);
>>
>> The function sets inode field, but don't change common_audit_data type.
>>
>> 		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
>> 	}
>>
>> So the dump_common_audit() function incorrectly interprets inode structure
>> as dentry, and Oops will happen.
>>
>> This patch reinitializes common_audit_data structures with correct type.
>> Also I removed unneeded
>> 	smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
>> initialization, because both dentry and inode pointers are stored
>> in the same union.
>>
>> Signed-off-by: Igor Zhbanov <i.zhbanov@...sung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
>> ---
>>   security/smack/smack_lsm.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index fa64740..d52c780 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -654,7 +654,7 @@ static int smack_inode_unlink(struct inode *dir, struct dentry *dentry)
>>   		/*
>>   		 * You also need write access to the containing directory
>>   		 */
>> -		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
>> +		smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
>>   		smk_ad_setfield_u_fs_inode(&ad, dir);
>>   		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
>>   	}
>> @@ -685,7 +685,7 @@ static int smack_inode_rmdir(struct inode *dir, struct dentry *dentry)
>>   		/*
>>   		 * You also need write access to the containing directory
>>   		 */
>> -		smk_ad_setfield_u_fs_path_dentry(&ad, NULL);
>> +		smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
>>   		smk_ad_setfield_u_fs_inode(&ad, dir);
>>   		rc = smk_curacc(smk_of_inode(dir), MAY_WRITE, &ad);
>>   	}

-- 
Best regards,
Igor Zhbanov,
Sub-Project Leader,
phone: +7 (495) 797 25 00 ext 3981
e-mail: i.zhbanov@...sung.com

Mobile group, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

--
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/