Le 22/12/2017 à 21:36, Craig Gallek a écrit :
> From: Craig Gallek <kr...@google.com>
>
> netns ids were added in commit 0c7aecd4bde4 and defined as signed
> integers in both the kernel datastructures and the netlink interface.
> However, the semantics of the implementation assume that the ids
> are always greater than or equal to zero, except for an internal
> sentinal value NETNSA_NSID_NOT_ASSIGNED.
>
> Several subsequent patches carried this pattern forward. This patch
> updates all of the netlink input paths of this value to enforce the
> 'greater than or equal to zero' constraint.
>
> This issue was discovered by syskaller. It would set a negative
> value for a netns id and then repeatedly call the RTM_GETLINK.
> The put_net call in that path would not trigger for negative netns ids,
> caused a reference count leak, and eventually overflowed. There are
> probably additional error paths that do not handle this situation
> correctly, but this was the only one I was able to trigger a real
> issue through.
>
> Fixes: 0c7aecd4bde4 ("netns: add rtnl cmd to add and get peer netns ids")
> Fixes: 317f4810e45e ("rtnl: allow to create device with IFLA_LINK_NETNSID
> set")
> Fixes: 79e1ad148c84 ("rtnetlink: use netnsid to query interface")
> CC: Jiri Benc <jb...@redhat.com>
> CC: Nicolas Dichtel <nicolas.dich...@6wind.com>
> CC: Jason A. Donenfeld <ja...@zx2c4.com>
> Signed-off-by: Craig Gallek <kr...@google.com>
> ---
> net/core/net_namespace.c | 2 ++
> net/core/rtnetlink.c | 17 +++++++++++++----
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 60a71be75aea..4b7ea33f5705 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -627,6 +627,8 @@ static int rtnl_net_newid(struct sk_buff *skb, struct
> nlmsghdr *nlh,
> return -EINVAL;
> }
> nsid = nla_get_s32(tb[NETNSA_NSID]);
> + if (nsid < 0)
> + return -EINVAL;
No, this breaks the current behavior.
Look at alloc_netid(). If reqid is < 0, the kernel allocates an nsid with no
constraint. If reqid is >= 0, it tries to alloc the specified nsid.
>
> if (tb[NETNSA_PID]) {
> peer = get_net_ns_by_pid(nla_get_u32(tb[NETNSA_PID]));
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index dabba2a91fc8..a928b8f081b8 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1733,10 +1733,12 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,
> struct netlink_callback *cb)
> ifla_policy, NULL) >= 0) {
> if (tb[IFLA_IF_NETNSID]) {
> netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
> - tgt_net = get_target_net(skb, netnsid);
> - if (IS_ERR(tgt_net)) {
> - tgt_net = net;
> - netnsid = -1;
> + if (netnsid >= 0) {
> + tgt_net = get_target_net(skb, netnsid);
I would prefer to put this test in get_target_net.
> + if (IS_ERR(tgt_net)) {
> + tgt_net = net;
> + netnsid = -1;
Maybe using NETNSA_NSID_NOT_ASSIGNED is better? Same for the initialization of
this variable.
> + }
> }
> }
>
> @@ -2792,6 +2794,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct
> nlmsghdr *nlh,
> if (tb[IFLA_LINK_NETNSID]) {
> int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>
> + if (id < 0) {
> + err = -EINVAL;
> + goto out;
> + }
> +
This is not needed. get_net_ns_by_id() returns NULL if id is < 0.
> link_net = get_net_ns_by_id(dest_net, id);
> if (!link_net) {
> err = -EINVAL;
> @@ -2883,6 +2890,8 @@ static int rtnl_getlink(struct sk_buff *skb, struct
> nlmsghdr *nlh,
>
> if (tb[IFLA_IF_NETNSID]) {
> netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
> + if (netnsid < 0)
> + return -EINVAL;
> tgt_net = get_target_net(skb, netnsid);
> if (IS_ERR(tgt_net))
> return PTR_ERR(tgt_net);
>
