netdev - Re: [PATCH] inet_diag: fix panic when unload inet

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]

Date:	Tue, 25 Sep 2012 08:36:51 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Gao feng <gaofeng@...fujitsu.com>
Cc:	davem@...emloft.net, stephen.hemminger@...tta.com, jengelh@...i.de,
	kuznet@....inr.ac.ru, netdev@...r.kernel.org
Subject: Re: [PATCH] inet_diag: fix panic when unload inet_diag

On Tue, 2012-09-25 at 10:48 +0800, Gao feng wrote:
> when inet_diag being compiled as module, inet_diag_handler_dump
> set netlink_dump_control.dump to inet_diag_dump,so if module
> inet_diag is unloaded,netlink will still try to call this function
> in netlink_dump. this will cause kernel panic.
> 
> fix this by adding a reference of inet_diag module before
> setting netlink_callback, and release this reference in
> netlink_callback.done.
> 
> Thanks for all help from Stephen,Jan and Eric.
...

>  
> @@ -1001,8 +1025,26 @@ static int inet_diag_handler_dump(struct sk_buff *skb, struct nlmsghdr *h)
>  		{
>  			struct netlink_dump_control c = {
>  				.dump = inet_diag_dump,
> +				.done = inet_diag_done,
>  			};
> -			return netlink_dump_start(net->diag_nlsk, skb, h, &c);
> +			int err;
> +			/*
> +			 * netlink_dump will call inet_diag_dump,
> +			 * so we need a reference of THIS_MODULE.
> +			 */
> +			if (!try_module_get(THIS_MODULE))
> +				return -EPROTONOSUPPORT;
> +
> +			err = netlink_dump_start(net->diag_nlsk, skb, h, &c);
> +
> +			if ((err != -EINTR) && (err != -ENOBUFS)) {
> +				/*
> +				 * netlink_callback set failed, release the
> +				 * referenct of THIS_MODULE.
> +				 */
> +				module_put(THIS_MODULE);
> +			}
> +			return err;
>  		}
>  	}
>  

Hmm... this seems error prone...

In the future, netlink_dump_start() could be changed to return other
errors than EINTR or ENOBUFS that need the module_put()

I would change netlink_dump_start() to __netlink_dump_start() and add a
module param to it, so that this module stuff is centralized in
__netlink_dump_start()

Then, instead of calling (from inet_diag)

netlink_dump_start(net->diag_nlsk, skb, nlh, &c);

you would use :

__netlink_dump_start(net->diag_nlsk, skb, nlh, &c, THIS_MODULE);

I wonder if this fix is not needed elsewhere eventually
(net/unix/af_unix.c for example ?)



--
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