On Wed, 2019-05-08 at 09:53 -0700, Eric Dumazet wrote: > > On 5/8/19 11:56 AM, Lukasz Pawelczyk wrote: > > On Wed, 2019-05-08 at 08:41 -0700, Eric Dumazet wrote: > > > On 5/8/19 11:25 AM, Lukasz Pawelczyk wrote: > > > > On Wed, 2019-05-08 at 07:58 -0700, Eric Dumazet wrote: > > > > > On 5/8/19 10:12 AM, Lukasz Pawelczyk wrote: > > > > > > The XT_SUPPL_GROUPS flag causes GIDs specified with > > > > > > XT_OWNER_GID to > > > > > > be also checked in the supplementary groups of a process. > > > > > > > > > > > > Signed-off-by: Lukasz Pawelczyk <[email protected]> > > > > > > --- > > > > > > include/uapi/linux/netfilter/xt_owner.h | 1 + > > > > > > net/netfilter/xt_owner.c | 23 > > > > > > ++++++++++++++++++++- > > > > > > -- > > > > > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/include/uapi/linux/netfilter/xt_owner.h > > > > > > b/include/uapi/linux/netfilter/xt_owner.h > > > > > > index fa3ad84957d5..d646f0dc3466 100644 > > > > > > --- a/include/uapi/linux/netfilter/xt_owner.h > > > > > > +++ b/include/uapi/linux/netfilter/xt_owner.h > > > > > > @@ -8,6 +8,7 @@ enum { > > > > > > XT_OWNER_UID = 1 << 0, > > > > > > XT_OWNER_GID = 1 << 1, > > > > > > XT_OWNER_SOCKET = 1 << 2, > > > > > > + XT_SUPPL_GROUPS = 1 << 3, > > > > > > }; > > > > > > > > > > > > struct xt_owner_match_info { > > > > > > diff --git a/net/netfilter/xt_owner.c > > > > > > b/net/netfilter/xt_owner.c > > > > > > index 46686fb73784..283a1fb5cc52 100644 > > > > > > --- a/net/netfilter/xt_owner.c > > > > > > +++ b/net/netfilter/xt_owner.c > > > > > > @@ -91,11 +91,28 @@ owner_mt(const struct sk_buff *skb, > > > > > > struct > > > > > > xt_action_param *par) > > > > > > } > > > > > > > > > > > > if (info->match & XT_OWNER_GID) { > > > > > > + unsigned int i, match = false; > > > > > > kgid_t gid_min = make_kgid(net->user_ns, info- > > > > > > > gid_min); > > > > > > kgid_t gid_max = make_kgid(net->user_ns, info- > > > > > > > gid_max); > > > > > > - if ((gid_gte(filp->f_cred->fsgid, gid_min) && > > > > > > - gid_lte(filp->f_cred->fsgid, gid_max)) ^ > > > > > > - !(info->invert & XT_OWNER_GID)) > > > > > > + struct group_info *gi = filp->f_cred- > > > > > > > group_info; > > > > > > + > > > > > > + if (gid_gte(filp->f_cred->fsgid, gid_min) && > > > > > > + gid_lte(filp->f_cred->fsgid, gid_max)) > > > > > > + match = true; > > > > > > + > > > > > > + if (!match && (info->match & XT_SUPPL_GROUPS) > > > > > > && gi) { > > > > > > + for (i = 0; i < gi->ngroups; ++i) { > > > > > > + kgid_t group = gi->gid[i]; > > > > > > + > > > > > > + if (gid_gte(group, gid_min) && > > > > > > + gid_lte(group, gid_max)) { > > > > > > + match = true; > > > > > > + break; > > > > > > + } > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + if (match ^ !(info->invert & XT_OWNER_GID)) > > > > > > return false; > > > > > > } > > > > > > > > > > > > > > > > > > > > > > How can this be safe on SMP ? > > > > > > > > > > > > > From what I see after the group_info rework some time ago this > > > > struct > > > > is never modified. It's replaced. Would > > > > get_group_info/put_group_info > > > > around the code be enough? > > > > > > What prevents the data to be freed right after you fetch filp- > > > > f_cred->group_info ? > > > > I think the get_group_info() I mentioned above would. group_info > > seems > > to always be freed by put_group_info(). > > The data can be freed _before_ get_group_info() is attempted. > > get_group_info() would do a use-after-free > > You would need something like RCU protection over this stuff, > this is not really only a netfilter change. >
sk_socket keeps reference to f_cred. f_cred keeps reference to group_info. As long as f_cred is alive and it doesn't seem to be the issue in the owner_mt() function, group_info should be alive as well as far as I can see. Its refcount will go down only when f_cred is freed (put_cred_rcu()). If there is something I'm missing please correct me. -- Lukasz Pawelczyk Samsung R&D Institute Poland Samsung Electronics
