netdev - RE: [patch] ixgbevf: potential NULL dereference on allocation failure

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, 14 Sep 2010 09:57:46 -0700
From:	"Rose, Gregory V" <gregory.v.rose@...el.com>
To:	David Miller <davem@...emloft.net>,
	"error27@...il.com" <error27@...il.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: RE: [patch] ixgbevf: potential NULL dereference on allocation
 failure

>-----Original Message-----
>From: David Miller [mailto:davem@...emloft.net]
>Sent: Friday, September 10, 2010 1:22 PM
>To: error27@...il.com
>Cc: Rose, Gregory V; Kirsher, Jeffrey T; netdev@...r.kernel.org; kernel-
>janitors@...r.kernel.org
>Subject: Re: [patch] ixgbevf: potential NULL dereference on allocation
>failure
>
>
>That's kind of crummy semantics, if any part fails we should unwind
>and return an error.  So just do the necessary memory allocations
>first, and don't make any changes unless they all succeed.
>
>This code also seems to be incredibly racy.  It allocates the new RING
>structure, and copies the existing entries over.  Meanwhile the chip
>is still running and we're potentially processing these same ring
>entries, so by the time we actually assign adapter->{rx,tx}_ring
>pointers the contents could have changed.
>

I've taken up your suggestions and implemented them (roughly) as suggested below.  After looking at the code I had to agree that it would be very confusing for a user to set new ring parameters, have the call partially succeed but get no error and then look at the parameters again and not see what he expected.  Now the code will do as suggested and just unwind all prior allocations and return an error if the new ring sizing didn't work.  The user will be left with the prior ring size allocations which is probably what he would expect.

The patch is going to be posted internally and after it goes through our review process it will be posted to netdev.

Regards and thanks for the suggestions,

- Greg

>Probably the simplest thing to do is to structure this such that the
>chip is quiesced around the entire ring set operation, so something
>like:
>
>	tx_ring = kcalloc();
>	if (!tx_ring)
>		goto do_err;
>	rx_ring = kcalloc();
>	if (!rx_ring)
>		goto rx_ring_free_err;
>
>	ixgbevf_down(adapter);
>
>	err = setup_tx_ring(adapter, tx_ring);
>	if (err)
>		goto device_up_err;
>	err = setup_rx_ring(adapter, rx_ring);
>	if (err)
>		goto device_up_err;
>
>	ixgbevf_up(adapter);
>
>	return 0;
>
>device_up_err:
>	tear_down_tx_ring(adapter, tx_ring);
>	tear_down_rx_ring(adapter, rx_ring);
>	kfree(tx_ring);
>rx_ring_free_err:
>	kfree(rx_ring);
>do_err:
>	return err;
--
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