netdev - Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1448971437.3330290.454583417.56E794A4@webmail.messagingengine.com>
Date:	Tue, 01 Dec 2015 13:03:57 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:	Pravin Shelar <pshelar@...ira.com>, Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH net v2] openvswitch: properly refcount vport-vxlan module

Hello Paolo,

On Mon, Nov 30, 2015, at 12:31, Paolo Abeni wrote:
> After 614732eaa12d, no refcount is maintained for the vport-vxlan module.
> This allows the userspace to remove such module while vport-vxlan
> devices still exist, which leads to later oops.
> 
> v1 -> v2:
>  - move vport 'owner' initialization in ovs_vport_ops_register()
>    and make such function a macro
> 
> Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  net/openvswitch/vport-geneve.c | 1 -
>  net/openvswitch/vport-gre.c    | 1 -
>  net/openvswitch/vport.c        | 4 ++--
>  net/openvswitch/vport.h        | 8 +++++++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/openvswitch/vport-geneve.c
> b/net/openvswitch/vport-geneve.c
> index efb736b..e41cd12 100644
> --- a/net/openvswitch/vport-geneve.c
> +++ b/net/openvswitch/vport-geneve.c
> @@ -117,7 +117,6 @@ static struct vport_ops ovs_geneve_vport_ops = {
>  	.destroy	= ovs_netdev_tunnel_destroy,
>  	.get_options	= geneve_get_options,
>  	.send		= dev_queue_xmit,
> -       .owner          = THIS_MODULE,
>  };
>  
>  static int __init ovs_geneve_tnl_init(void)
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index c3257d7..7f8897f 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -89,7 +89,6 @@ static struct vport_ops ovs_gre_vport_ops = {
>  	.create		= gre_create,
>  	.send		= dev_queue_xmit,
>  	.destroy	= ovs_netdev_tunnel_destroy,
> -       .owner          = THIS_MODULE,
>  };
>  
>  static int __init ovs_gre_tnl_init(void)
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index e194c10a..31cbc8c 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -71,7 +71,7 @@ static struct hlist_head *hash_bucket(const struct net
> *net, const char *name)
>  	return &dev_table[hash & (VPORT_HASH_BUCKETS - 1)];
>  }
>  
> -int ovs_vport_ops_register(struct vport_ops *ops)
> +int __ovs_vport_ops_register(struct vport_ops *ops)
>  {
>  	int err = -EEXIST;
>  	struct vport_ops *o;
> @@ -87,7 +87,7 @@ errout:
>  	ovs_unlock();
>  	return err;
>  }
> -EXPORT_SYMBOL_GPL(ovs_vport_ops_register);
> +EXPORT_SYMBOL_GPL(__ovs_vport_ops_register);
>  
>  void ovs_vport_ops_unregister(struct vport_ops *ops)
>  {
> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> index bdfd82a..8ea3a96 100644
> --- a/net/openvswitch/vport.h
> +++ b/net/openvswitch/vport.h
> @@ -196,7 +196,13 @@ static inline const char *ovs_vport_name(struct
> vport *vport)
>  	return vport->dev->name;
>  }
>  
> -int ovs_vport_ops_register(struct vport_ops *ops);
> +int __ovs_vport_ops_register(struct vport_ops *ops);
> +#define ovs_vport_ops_register(ops)            \
> +       ({                                      \
> +               (ops)->owner = THIS_MODULE;     \
> +               __ovs_vport_ops_register(ops);  \
> +       })
> +
>  void ovs_vport_ops_unregister(struct vport_ops *ops);

This doesn't look correct (the fault is not your patch).

The register function adds the list_head of the ops struct on a list
without increasing the module reference counter. Thus the module could
be removed and the list walk would walk across the unloaded data section
left over by the module. Does a kmemdup make sense here and a later free
in the unregister function? We test type first and then only touch
functions after getting module refcnt, so it would appear safe to me
then.

Thanks,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html