netdev - Re: [PATCH] bonding: fix error handling if slave is busy

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:	Sat, 31 Dec 2011 17:11:05 +0100
From:	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>
To:	Stephen Hemminger <shemminger@...tta.com>
CC:	David Miller <davem@...emloft.net>,
	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] bonding: fix error handling if slave is busy

Le 30/12/2011 23:40, Stephen Hemminger a écrit :
> The bonding device can cause kernel panic in the enslave error handling.
>
> If slave device already has a receive handler registered, then the
> error unwind does not clear the new entry out of the slave list.
> This ends up leaving a reference to freed memory in the bond
> device slave linked list.
>
> The following is a simple example:
> # modprobe dummy
> # ip li add dummy0-1 link dummy0 type macvlan
> # modprobe bonding
> # echo +dummy0>/sys/class/net/bond0/bonding/slaves
> # ip -s li show dev bond0
>
> This returns with -EBUSY, but the bonding device has bogus entry in
> the slave list, and will panic on next operation that gets statistics
> from bond0.
>
> The fix is to detach the slave (which removes it from the list)
> in the unwind path.
>
>
> Signed-off-by: Stephen Hemminger<shemminger@...tta.com>
>
> ---
> Patch is against net-next but should be applied to net (3.2), and
> stable (3.1 and 3.0).
>
> --- a/drivers/net/bonding/bond_main.c	2011-12-30 14:20:03.171823181 -0800
> +++ b/drivers/net/bonding/bond_main.c	2011-12-30 14:20:20.232020474 -0800
> @@ -1853,6 +1853,9 @@ err_dest_symlinks:
>   	bond_destroy_slave_symlinks(bond_dev, slave_dev);
>
>   err_close:
> +	write_lock_bh(&bond->lock);
> +	bond_detach_slave(bond, new_slave);
> +	write_unlock_bh(&bond->lock);
>   	dev_close(slave_dev);
>
>   err_unset_master:

NAK.

There are three 'goto err_close' before the call to bond_attach_slave. For those three goto, your 
path will call bond_detach_slave without a previous call to bond_attach_slave.

This would at least decrement bond->slave_cnt, without having incremented it before.

Do I miss something ?

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