From: CHANDAN VN <chandan.vn@samsung.com>
To: Tejun Heo <tj@kernel.org>, Casey Schaufler <casey@schaufler-ca.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"bfields@fieldses.org" <bfields@fieldses.org>,
"jlayton@kernel.org" <jlayton@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
CPGS <cpgs@samsung.com>,
Sireesha Talluri <sireesha.t@samsung.com>,
Chris Wright <chrisw@sous-sol.org>,
"linux-security-module@vger.kernel.org"
<linux-security-module@vger.kernel.org>
Subject: RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
Date: Fri, 01 Jun 2018 14:26:09 +0530 [thread overview]
Message-ID: <20180601085609epcms5p5fefac0156a4816e9e48751211ab595ee@epcms5p5> (raw)
In-Reply-To: <1ced6bce-92cc-7e0c-fab4-0aaa3d03b82f@schaufler-ca.com>
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.
WARNING: multiple messages have this Message-ID (diff)
From: CHANDAN VN <chandan.vn@samsung.com>
To: Tejun Heo <tj@kernel.org>, Casey Schaufler <casey@schaufler-ca.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"bfields@fieldses.org" <bfields@fieldses.org>,
"jlayton@kernel.org" <jlayton@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
CPGS <cpgs@samsung.com>,
Sireesha Talluri <sireesha.t@samsung.com>,
Chris Wright <chrisw@sous-sol.org>,
"linux-security-module@vger.kernel.org"
<linux-security-module@vger.kernel.org>
Subject: RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
Date: Fri, 01 Jun 2018 14:26:09 +0530 [thread overview]
Message-ID: <20180601085609epcms5p5fefac0156a4816e9e48751211ab595ee@epcms5p5> (raw)
In-Reply-To: <1ced6bce-92cc-7e0c-fab4-0aaa3d03b82f@schaufler-ca.com>
Hi
=C2=A0=0D=0A=0D=0A>On=C2=A05/31/2018=C2=A09:11=C2=A0AM,=C2=A0Tejun=C2=A0Heo=
=C2=A0wrote:=0D=0A>=C2=A0On=C2=A0Thu,=C2=A0May=C2=A031,=C2=A02018=C2=A0at=
=C2=A009:04:25AM=C2=A0-0700,=C2=A0Casey=C2=A0Schaufler=C2=A0wrote:=0D=0A>>>=
=C2=A0On=C2=A05/31/2018=C2=A08:39=C2=A0AM,=C2=A0Tejun=C2=A0Heo=C2=A0wrote:=
=0D=0A>>>>=C2=A0(cc'ing=C2=A0more=C2=A0security=C2=A0folks=C2=A0and=C2=A0co=
pying=C2=A0whole=C2=A0body)=0D=0A>>>>=0D=0A>>>>=C2=A0So,=C2=A0I'm=C2=A0sure=
=C2=A0the=C2=A0patch=C2=A0fixes=C2=A0the=C2=A0memory=C2=A0leak=C2=A0but=C2=
=A0API=C2=A0wise=C2=A0it=C2=A0looks=0D=0A>>>>=C2=A0super=C2=A0confusing.=C2=
=A0=C2=A0Can=C2=A0security=C2=A0folks=C2=A0chime=C2=A0in=C2=A0here?=C2=A0=
=C2=A0Is=C2=A0this=C2=A0the=C2=A0right=0D=0A>>>>=C2=A0fix?=0D=0A>>>>=C2=A0s=
ecurity_inode_getsecctx()=C2=A0provides=C2=A0a=C2=A0security=C2=A0context.=
=C2=A0Technically,=0D=0A>>>>=C2=A0this=C2=A0is=C2=A0a=C2=A0data=C2=A0blob,=
=C2=A0although=C2=A0both=C2=A0provider=C2=A0provide=C2=A0a=C2=A0null=C2=A0t=
erminated=0D=0A>>>>=C2=A0string.=C2=A0security_inode_getsecurity(),=C2=A0on=
=C2=A0the=C2=A0other=C2=A0hand,=C2=A0provides=C2=A0a=0D=0A>>>>=C2=A0string=
=C2=A0to=C2=A0match=C2=A0an=C2=A0attribute=C2=A0name.=C2=A0The=C2=A0former=
=C2=A0releases=C2=A0the=C2=A0security=0D=0A>>>>=C2=A0context=C2=A0with=C2=
=A0security_release_secctx(),=C2=A0where=C2=A0the=C2=A0later=C2=A0releases=
=C2=A0the=0D=0A>>>>=C2=A0string=C2=A0with=C2=A0kfree().=0D=0A>>>>=0D=0A>>>>=
=C2=A0When=C2=A0the=C2=A0Smack=C2=A0hook=C2=A0smack_inode_getsecctx()=C2=A0=
was=C2=A0added=C2=A0in=C2=A02009=0D=0A>>>>=C2=A0for=C2=A0use=C2=A0by=C2=A0l=
abeled=C2=A0NFS=C2=A0the=C2=A0alloc=C2=A0value=C2=A0passed=C2=A0to=0D=0A>>>=
=C2=A0smack_inode_getsecurity()=C2=A0was=C2=A0set=C2=A0incorrectly.=C2=A0Th=
is=C2=A0wasn't=C2=A0a=0D=0A>>>=C2=A0major=C2=A0issue,=C2=A0since=C2=A0label=
ed=C2=A0NFS=C2=A0is=C2=A0a=C2=A0fringe=C2=A0case.=C2=A0When=C2=A0kernfs=0D=
=0A>>>=C2=A0started=C2=A0using=C2=A0the=C2=A0hook,=C2=A0it=C2=A0became=C2=
=A0the=C2=A0issue=C2=A0you=C2=A0discovered.=0D=0A>>>=0D=0A>>>=C2=A0The=C2=
=A0reason=C2=A0that=C2=A0we=C2=A0have=C2=A0all=C2=A0this=C2=A0confusion=C2=
=A0is=C2=A0that=C2=A0SELinux=0D=0A>>>=C2=A0generates=C2=A0security=C2=A0con=
texts=C2=A0as=C2=A0needed,=C2=A0while=C2=A0Smack=C2=A0keeps=C2=A0them=0D=0A=
>>>=C2=A0around=C2=A0all=C2=A0the=C2=A0time.=C2=A0Releasing=C2=A0an=C2=A0SE=
Linux=C2=A0context=C2=A0frees=C2=A0memory,=0D=0A>>>=C2=A0while=C2=A0releasi=
ng=C2=A0a=C2=A0Smack=C2=A0context=C2=A0is=C2=A0a=C2=A0null=C2=A0operation.=
=0D=0A>>=C2=A0Any=C2=A0chance=C2=A0this=C2=A0detail=C2=A0can=C2=A0be=C2=A0h=
idden=C2=A0behind=C2=A0security=C2=A0api?=C2=A0=C2=A0This=C2=A0looks=0D=0A>=
>=C2=A0pretty=C2=A0error-prone,=C2=A0no?=0D=0A=C2=A0=0D=0A>>It=C2=A0*is*=C2=
=A0hidden=C2=A0behind=C2=A0the=C2=A0security=C2=A0API.=C2=A0The=C2=A0proble=
m=C2=A0is=C2=A0strictly=0D=0A>>within=C2=A0the=C2=A0Smack=C2=A0code,=C2=A0w=
here=C2=A0the=C2=A0implementer=C2=A0of=C2=A0smack_inode_getsecctx()=0D=0A>>=
made=C2=A0an=C2=A0error.=0D=0A=0D=0AI=20agree=20that=20the=20fix=20can=20be=
=20done=20simply=20by=20using=20=22false=22=20for=20=0D=0Asmack_inode_getse=
curity(),=20but=20what=20happens=20with=20kernfs_node_setsecdata()=0D=0Aand=
=20smack_inode_notifysecctx().=20kernfs_node_setsecdata()=20is=20probably=
=20ignorable=0D=0Abut=20smack_inode_notifysecctx()=20is=20sending=20the=20=
=22ctx=22=20to=20smack_inode_setsecurity()=0D=0Aand=20since=20=22ctx=22=20w=
ould=20be=20NULL=20because=20we=20used=20=22false=22,=20smack_inode_setsecu=
rity()=0D=0Abecomes=20dummy.=0D=0A=0D=0A
WARNING: multiple messages have this Message-ID (diff)
From: chandan.vn@samsung.com (CHANDAN VN)
To: linux-security-module@vger.kernel.org
Subject: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
Date: Fri, 01 Jun 2018 14:26:09 +0530 [thread overview]
Message-ID: <20180601085609epcms5p5fefac0156a4816e9e48751211ab595ee@epcms5p5> (raw)
In-Reply-To: <1ced6bce-92cc-7e0c-fab4-0aaa3d03b82f@schaufler-ca.com>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-06-01 8:56 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcas1p2.samsung.com>
2018-05-31 9:28 ` [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set CHANDAN VN
2018-05-31 15:26 ` Casey Schaufler
2018-05-31 20:57 ` Eric W. Biederman
2018-05-31 21:08 ` Casey Schaufler
2018-05-31 15:39 ` Tejun Heo
2018-05-31 15:39 ` Tejun Heo
2018-05-31 16:04 ` Casey Schaufler
2018-05-31 16:04 ` Casey Schaufler
2018-05-31 16:11 ` Tejun Heo
2018-05-31 16:11 ` Tejun Heo
2018-05-31 16:22 ` Casey Schaufler
2018-05-31 16:22 ` Casey Schaufler
[not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5>
2018-06-01 8:56 ` CHANDAN VN [this message]
2018-06-01 8:56 ` CHANDAN VN
2018-06-01 8:56 ` CHANDAN VN
2018-06-01 16:22 ` Casey Schaufler
2018-06-01 16:22 ` Casey Schaufler
[not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7>
2018-06-01 16:29 ` CHANDAN VN
2018-06-01 16:29 ` CHANDAN VN
2018-06-01 16:29 ` CHANDAN VN
2018-06-01 16:41 ` Casey Schaufler
2018-06-01 16:41 ` Casey Schaufler
2018-06-01 17:45 ` [PATCH] Smack: Fix memory leak in smack_inode_getsecctx Casey Schaufler
2018-06-01 17:45 ` Casey Schaufler
2018-06-04 21:01 ` Casey Schaufler
2018-06-04 21:01 ` Casey Schaufler
2018-06-04 21:27 ` Tejun Heo
2018-06-04 21:27 ` Tejun Heo
[not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3>
2018-06-05 7:04 ` CHANDAN VN
2018-06-05 7:04 ` CHANDAN VN
2018-06-05 7:04 ` CHANDAN VN
2018-06-05 14:29 ` Casey Schaufler
[not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p4>
2018-06-05 14:46 ` CHANDAN VN
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180601085609epcms5p5fefac0156a4816e9e48751211ab595ee@epcms5p5 \
--to=chandan.vn@samsung.com \
--cc=bfields@fieldses.org \
--cc=casey@schaufler-ca.com \
--cc=chrisw@sous-sol.org \
--cc=cpgs@samsung.com \
--cc=gregkh@linuxfoundation.org \
--cc=jlayton@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=sireesha.t@samsung.com \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.