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

[CHANDAN VN]

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.