[BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Jean Delvare]

[Please CC: me on reply, I'm not subscribed (yet)]

Hi all,

I just found two very weird memsets in drivers/net/sk_mca.c. I am not
working on that driver at all, but some grepping of the kernel source
pointed me to it so I thought I wouldn't keep quiet about it.

Here is the offending code:

static void skmca_set_multicast_list(struct net_device *dev)
{
	(...)
	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
		memset(block.LAdrF, 8, 0xff);
	} else {		/* get selected/no multicasts */
		(...)
		memset(block.LAdrF, 8, 0x00);
		(...)
	}
	(...)
}

Is it just me, or are these two memsets just plain broken? Not only the
size is hardcoded, but the parameters are swapped! I guess that this
driver never worked in multicast mode. The correct memsets are
obviously:
		memset(block.LAdrF, 0xff, sizeof(block.LAdrF));
and
		memset(block.LAdrF, 0x00, sizeof(block.LAdrF));
respectively.

The odd thing is that the bug is there since the driver was introduced
in the 2.2.10 kernel. So, 2.2, 2.4 and 2.6 kernels are all affected.

I admit I'm a bit surprized that this could survive until today, so just
tell me if it's me missing the point ;)

Thanks.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Re: [BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Andrew Morton]

Jean Delvare <khali@linux-fr.org> wrote:
>
> I just found two very weird memsets in drivers/net/sk_mca.c.

yup.


--- 25/drivers/net/sk_mca.c~sk_mca-multicast-fix	2004-04-10 01:37:06.739989760 -0700
+++ 25-akpm/drivers/net/sk_mca.c	2004-04-10 01:38:33.684772144 -0700
@@ -996,14 +996,12 @@ static void skmca_set_multicast_list(str
 	else
 		block.Mode &= ~LANCE_INIT_PROM;
 
-	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
-		memset(block.LAdrF, 8, 0xff);
-	} else {		/* get selected/no multicasts */
-
+	memset(block.LAdrF, 0xff, sizeof(block.LAdrF));
+	if (!(dev->flags & IFF_ALLMULTI)) {
+		/* get selected/no multicasts */
 		struct dev_mc_list *mptr;
 		int code;
 
-		memset(block.LAdrF, 8, 0x00);
 		for (mptr = dev->mc_list; mptr != NULL; mptr = mptr->next) {
 			code = GetHash(mptr->dmi_addr);
 			block.LAdrF[(code >> 3) & 7] |= 1 << (code & 7);

_

Re: [BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Russell King]

On Sat, Apr 10, 2004 at 01:40:40AM -0700, Andrew Morton wrote:
> Jean Delvare <khali@linux-fr.org> wrote:
> > I just found two very weird memsets in drivers/net/sk_mca.c.
> 
> yup.

Erm...

> --- 25/drivers/net/sk_mca.c~sk_mca-multicast-fix	2004-04-10 01:37:06.739989760 -0700
> +++ 25-akpm/drivers/net/sk_mca.c	2004-04-10 01:38:33.684772144 -0700
> @@ -996,14 +996,12 @@ static void skmca_set_multicast_list(str
>  	else
>  		block.Mode &= ~LANCE_INIT_PROM;
>  
> -	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
> -		memset(block.LAdrF, 8, 0xff);
> -	} else {		/* get selected/no multicasts */
> -
> +	memset(block.LAdrF, 0xff, sizeof(block.LAdrF));

Initialise the whole of block.LAdrF to all-bits-set, and then...

> +	if (!(dev->flags & IFF_ALLMULTI)) {
> +		/* get selected/no multicasts */
>  		struct dev_mc_list *mptr;
>  		int code;
>  
> -		memset(block.LAdrF, 8, 0x00);
>  		for (mptr = dev->mc_list; mptr != NULL; mptr = mptr->next) {
>  			code = GetHash(mptr->dmi_addr);
>  			block.LAdrF[(code >> 3) & 7] |= 1 << (code & 7);

Set bits, which are already set from the previous memset.

Surely this can't be right?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Andrew Morton]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
> Erm...

--- 25/drivers/net/sk_mca.c~sk_mca-multicast-fix	2004-04-10 01:42:29.464928112 -0700
+++ 25-akpm/drivers/net/sk_mca.c	2004-04-10 01:57:20.106530008 -0700
@@ -997,13 +997,13 @@ static void skmca_set_multicast_list(str
 		block.Mode &= ~LANCE_INIT_PROM;
 
 	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
-		memset(block.LAdrF, 8, 0xff);
+		memset(block.LAdrF, 0xff, sizeof(block.LAdrF));
 	} else {		/* get selected/no multicasts */
 
 		struct dev_mc_list *mptr;
 		int code;
 
-		memset(block.LAdrF, 8, 0x00);
+		memset(block.LAdrF, 0, sizeof(block.LAdrF));
 		for (mptr = dev->mc_list; mptr != NULL; mptr = mptr->next) {
 			code = GetHash(mptr->dmi_addr);
 			block.LAdrF[(code >> 3) & 7] |= 1 << (code & 7);

_

Re: [BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Denis Vlasenko]

On Saturday 10 April 2004 11:20, Jean Delvare wrote:
> [Please CC: me on reply, I'm not subscribed (yet)]
>
> Hi all,
>
> I just found two very weird memsets in drivers/net/sk_mca.c. I am not
> working on that driver at all, but some grepping of the kernel source
> pointed me to it so I thought I wouldn't keep quiet about it.
>
> Here is the offending code:
>
> static void skmca_set_multicast_list(struct net_device *dev)
> {
> 	(...)
> 	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
> 		memset(block.LAdrF, 8, 0xff);
> 	} else {		/* get selected/no multicasts */
> 		(...)
> 		memset(block.LAdrF, 8, 0x00);
> 		(...)
> 	}
> 	(...)
> }
>
> Is it just me, or are these two memsets just plain broken? Not only the
> size is hardcoded, but the parameters are swapped! I guess that this
> driver never worked in multicast mode. The correct memsets are
> obviously:
> 		memset(block.LAdrF, 0xff, sizeof(block.LAdrF));
> and
> 		memset(block.LAdrF, 0x00, sizeof(block.LAdrF));
> respectively.
>
> The odd thing is that the bug is there since the driver was introduced
> in the 2.2.10 kernel. So, 2.2, 2.4 and 2.6 kernels are all affected.
>
> I admit I'm a bit surprized that this could survive until today, so just
> tell me if it's me missing the point ;)

Good catch!

No, I don't think you're missing the point. There are lots of drivers,
and unlike core kernel code, drivers have corner cases which are never
tested. Work on drivers is always welcome.

Feel free to make patch and submit it to Jeff Garzik <jgarzik@pobox.com>.
For 2.2 and 2.4, I think you can CC kernel maintainers too.
--
vda

Re: [BUG 2.2/2.4/2.6] broken memsets in net/sk_mca.c (multicast)

[Jean Delvare]

> --- 25/drivers/net/sk_mca.c~sk_mca-multicast-fix	2004-04-10 01:42:29.464928112 -0700
> +++ 25-akpm/drivers/net/sk_mca.c	2004-04-10 01:57:20.106530008 -0700
> @@ -997,13 +997,13 @@ static void skmca_set_multicast_list(str
>  		block.Mode &= ~LANCE_INIT_PROM;
>  
>  	if (dev->flags & IFF_ALLMULTI) {	/* get all multicasts */
> -		memset(block.LAdrF, 8, 0xff);
> +		memset(block.LAdrF, 0xff, sizeof(block.LAdrF));
>  	} else {		/* get selected/no multicasts */
>  
>  		struct dev_mc_list *mptr;
>  		int code;
>  
> -		memset(block.LAdrF, 8, 0x00);
> +		memset(block.LAdrF, 0, sizeof(block.LAdrF));
>  		for (mptr = dev->mc_list; mptr != NULL; mptr = mptr->next) {
>  			code = GetHash(mptr->dmi_addr);
>  			block.LAdrF[(code >> 3) & 7] |= 1 << (code & 7);
> 

Looks better ;)

While you're at it, please consider the following misc changes:

1* In net/sk_mca.h:
#define CSR3_BSWAP_OFF     0	/* Bit 2 = 0 -> no byte swap         */
#define CSR3_BSWAP_ON      0	/* Bit 2 = 1 -> byte swap            */
The second define should obviously read 4, not 0. It's harmess because
the define isn't used anywhere, but still...

2* Alfred Arnold's alternate e-mail address, aarnold at elsa.de, looks
dead. Maybe you could remove it from the two drivers it appears in:
net/ibmlana.c and net/sk_mca.c. The first address still works. Quoting
Alfred: "ELSA (my old employer) went bankrupt about two years ago, you
may remove this address or replace it with my current work address:
alfred.arnold at lancom.de".

Thanks.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/