Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[Casey Schaufler]

On 6/1/2018 1:56 AM, CHANDAN VN wrote:
> Hi
> Â
>
>> OnÂ5/31/2018Â9:11ÂAM,ÂTejunÂHeoÂwrote:
>> ÂOnÂThu,ÂMayÂ31,Â2018ÂatÂ09:04:25AMÂ-0700,ÂCaseyÂSchauflerÂwrote:
>>>> ÂOnÂ5/31/2018Â8:39ÂAM,ÂTejunÂHeoÂwrote:
>>>>> Â(cc'ingÂmoreÂsecurityÂfolksÂandÂcopyingÂwholeÂbody)
>>>>>
>>>>> ÂSo,ÂI'mÂsureÂtheÂpatchÂfixesÂtheÂmemoryÂleakÂbutÂAPIÂwiseÂitÂlooks
>>>>> ÂsuperÂconfusing.ÂÂCanÂsecurityÂfolksÂchimeÂinÂhere?ÂÂIsÂthisÂtheÂright
>>>>> Âfix?
>>>>> Âsecurity_inode_getsecctx()ÂprovidesÂaÂsecurityÂcontext.ÂTechnically,
>>>>> ÂthisÂisÂaÂdataÂblob,ÂalthoughÂbothÂproviderÂprovideÂaÂnullÂterminated
>>>>> Âstring.Âsecurity_inode_getsecurity(),ÂonÂtheÂotherÂhand,ÂprovidesÂa
>>>>> ÂstringÂtoÂmatchÂanÂattributeÂname.ÂTheÂformerÂreleasesÂtheÂsecurity
>>>>> ÂcontextÂwithÂsecurity_release_secctx(),ÂwhereÂtheÂlaterÂreleasesÂthe
>>>>> ÂstringÂwithÂkfree().
>>>>>
>>>>> ÂWhenÂtheÂSmackÂhookÂsmack_inode_getsecctx()ÂwasÂaddedÂinÂ2009
>>>>> ÂforÂuseÂbyÂlabeledÂNFSÂtheÂallocÂvalueÂpassedÂto
>>>> Âsmack_inode_getsecurity()ÂwasÂsetÂincorrectly.ÂThisÂwasn'tÂa
>>>> ÂmajorÂissue,ÂsinceÂlabeledÂNFSÂisÂaÂfringeÂcase.ÂWhenÂkernfs
>>>> ÂstartedÂusingÂtheÂhook,ÂitÂbecameÂtheÂissueÂyouÂdiscovered.
>>>>
>>>> ÂTheÂreasonÂthatÂweÂhaveÂallÂthisÂconfusionÂisÂthatÂSELinux
>>>> ÂgeneratesÂsecurityÂcontextsÂasÂneeded,ÂwhileÂSmackÂkeepsÂthem
>>>> ÂaroundÂallÂtheÂtime.ÂReleasingÂanÂSELinuxÂcontextÂfreesÂmemory,
>>>> ÂwhileÂreleasingÂaÂSmackÂcontextÂisÂaÂnullÂoperation.
>>> ÂAnyÂchanceÂthisÂdetailÂcanÂbeÂhiddenÂbehindÂsecurityÂapi?ÂÂThisÂlooks
>>> ÂprettyÂerror-prone,Âno?
> Â
>>> ItÂ*is*ÂhiddenÂbehindÂtheÂsecurityÂAPI.ÂTheÂproblemÂisÂstrictly
>>> withinÂtheÂSmackÂcode,ÂwhereÂtheÂimplementerÂofÂsmack_inode_getsecctx()
>>> madeÂanÂerror.
> I agree that the fix can be done simply by using "false" for 
> smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()
> and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable
> but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()
> and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()
> becomes dummy.

Thank you for pointing this out. You're right, there's more
at issue here than changing the alloc flag will fix. I think
that calling smack_inode_getsecurity() from smack_inode_getsecctx()
is making the code more complicated than it needs to be. I will
have a patch shortly.