From: Eric Paris <eparis@redhat.com>
To: Paul Moore <paul.moore@hp.com>
Cc: linux-kernel@vger.kernel.org, netfilter-devel@vger.kernel.org,
netfilter@vger.kernel.org, jmorris@namei.org,
selinux@tycho.nsa.gov, sds@tycho.nsa.gov, jengelh@medozas.de,
linux-security-module@vger.kernel.org,
mr.dash.four@googlemail.com, pablo@netfilter.org
Subject: Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink
Date: Tue, 12 Oct 2010 19:24:51 -0400 [thread overview]
Message-ID: <1286925891.2614.13.camel@localhost.localdomain> (raw)
In-Reply-To: <1286925284.5133.98.camel@sifl>
On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > The conntrack code can export the internal secid to userspace. These are
> > dynamic, can change on lsm changes, and have no meaning in userspace. We
> > should instead be sending lsm contexts to userspace instead. This patch sends
> > the secctx (rather than secid) to userspace over the netlink socket. We use a
> > new field CTA_SECCTX and stop using the the old CTA_SECMARK field since it did
> > not send particularly useful information.
>
> Looks fine to me in principal, just a nit-picky comment below ...
>
> > -ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
> > +ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> > {
> > - NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark));
> > - return 0;
> > + struct nlattr *nest_secctx;
> > + int len, ret;
> > + char *secctx;
> > +
> > + ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> > + if (ret)
> > + return ret;
> > +
> > + ret = -1;
> > + nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> > + if (!nest_secctx)
> > + goto nla_put_failure;
> >
> > + NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> > + nla_nest_end(skb, nest_secctx);
> > +
> > + ret = 0;
> > nla_put_failure:
> > - return -1;
> > + security_release_secctx(secctx, len);
> > + return ret;
> > }
>
> All the ret assignments bother me, I also don't think "nla_put_failure"
> is a good choice since we run this code both on success and failure -
> how about this:
#define NLA_PUT(skb, attrtype, attrlen, data) \
do { \
if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
goto nla_put_failure; \
} while(0)
#define NLA_PUT_STRING(skb, attrtype, value) \
NLA_PUT(skb, attrtype, strlen(value) + 1, value)
so we can't get rid of the nla_put_failure tag and it also means your
ret values aren't quite right, we have to set ret = -1 before the
NLA_PUT_STRING()....
> ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> struct nlattr *nest_secctx;
> int len, ret;
> char *secctx;
>
> ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> if (ret)
> return ret;
>
> nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> if (!nest_secctx) {
> ret = -1;
> goto dump_secctx_out;
> }
>
> NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> nla_nest_end(skb, nest_secctx);
>
> dump_secctx_out:
> security_release_secctx(secctx, len);
> return ret;
> }
>
next prev parent reply other threads:[~2010-10-12 23:24 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-12 15:40 [PATCH 1/5] secmark: do not return early if there was no error Eric Paris
2010-10-12 15:40 ` [PATCH 2/5] secmark: make secmark object handling generic Eric Paris
2010-10-12 16:26 ` Jan Engelhardt
2010-10-12 17:24 ` Pablo Neira Ayuso
2010-10-12 17:45 ` Eric Paris
2010-10-12 17:53 ` Jan Engelhardt
2010-10-12 18:15 ` Pablo Neira Ayuso
2010-10-12 22:55 ` Paul Moore
2010-10-12 23:01 ` Eric Paris
2010-10-12 23:06 ` Paul Moore
2010-10-12 23:14 ` Eric Paris
2010-10-12 23:20 ` Paul Moore
2010-10-12 15:40 ` [PATCH 3/5] security: secid_to_secctx returns len when data is NULL Eric Paris
2010-10-12 23:04 ` Paul Moore
2010-10-12 15:40 ` [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink Eric Paris
2010-10-12 23:14 ` Paul Moore
2010-10-12 23:24 ` Eric Paris [this message]
2010-10-13 13:41 ` Paul Moore
2010-10-12 15:40 ` [PATCH 5/5] secmark: export secctx, drop secmark in procfs Eric Paris
2010-10-12 23:19 ` Paul Moore
2010-10-12 23:27 ` Eric Paris
2010-10-12 22:52 ` [PATCH 1/5] secmark: do not return early if there was no error Paul Moore
2010-10-12 23:38 ` James Morris
2010-10-12 23:50 ` Eric Paris
2010-10-13 4:07 ` James Morris
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=1286925891.2614.13.camel@localhost.localdomain \
--to=eparis@redhat.com \
--cc=jengelh@medozas.de \
--cc=jmorris@namei.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mr.dash.four@googlemail.com \
--cc=netfilter-devel@vger.kernel.org \
--cc=netfilter@vger.kernel.org \
--cc=pablo@netfilter.org \
--cc=paul.moore@hp.com \
--cc=sds@tycho.nsa.gov \
--cc=selinux@tycho.nsa.gov \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).