atp870u: 64 bit bug in probe()

On 64 bit CPUs there is a memory corruption bug on probe(). It should be setting 32 bits of data on both 32 and 64 bit.

---
Static checker stuff. I can't test this.

index 15a629d..3edbf30 100644
@@ -2791,11 +2791,11 @@ next_fblk_885:
 		    p->global_map[m]= 0;
 		    for (k=0; k < 4; k++) {
 			outw(n++,base_io + 0x3c);
-			((unsigned long *)&setupdata[m][0])[k]=inl(base_io + 0x38);
+			((u32 *)&setupdata[m][0])[k]=inl(base_io + 0x38);
 		    }
 		    for (k=0; k < 4; k++) {
 			outw(n++,base_io + 0x3c);
-			((unsigned long *)&p->sp[m][0])[k]=inl(base_io + 0x38);
+			((u32 *)&p->sp[m][0])[k]=inl(base_io + 0x38);
 		    }
 		    n += 8;
 		}

On 64 bit CPUs there is a memory corruption bug on probe(). It should be a u32 pointer instead of an unsigned long pointer or we write past the end of the setupdata[] array.

---
Someone reported in 2003 that probe has a NULL deref so maybe it's related to this memory corruption?
https://bugzilla.kernel.org/show_bug.cgi?id18

If only we had applied this patch when I originally sent it two years ago, then it would only be 10 years too late instead of 12! :P

index 05301bc..62acabd 100644
@@ -2791,11 +2791,11 @@ next_fblk_885:
 		    p->global_map[m]= 0;
 		    for (k=0; k < 4; k++) {
 			outw(n++,base_io + 0x3c);
-			((unsigned long *)&setupdata[m][0])[k]=inl(base_io + 0x38);
+			((u32 *)&setupdata[m][0])[k]=inl(base_io + 0x38);
 		    }
 		    for (k=0; k < 4; k++) {
 			outw(n++,base_io + 0x3c);
-			((unsigned long *)&p->sp[m][0])[k]=inl(base_io + 0x38);
+			((u32 *)&p->sp[m][0])[k]=inl(base_io + 0x38);
 		    }
 		    n += 8;
 		}

Cheers,

Hannes

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 64 bit CPUs there is a memory corruption bug on probe(). It should be a u32 pointer instead of an unsigned long pointer or we write past the end of the setupdata[] array.

---
Resending because we have shuffled the code around so the patch needed to be refreshed against linux-next. Although I do wonder why we are still working on this code since it has never worked on 64 bit systems so probably all the users gave up a decade ago.

index 8b52a9d..b46997c 100644
@@ -1413,11 +1413,11 @@ static void atp885_init(struct Scsi_Host *shpnt)
 			atpdev->global_map[m] = 0;
 			for (k = 0; k < 4; k++) {
 				atp_writew_base(atpdev, 0x3c, n++);
-				((unsigned long *)&setupdata[m][0])[k] = atp_readl_base(atpdev, 0x38);
+				((u32 *)&setupdata[m][0])[k] = atp_readl_base(atpdev, 0x38);
 			}
 			for (k = 0; k < 4; k++) {
 				atp_writew_base(atpdev, 0x3c, n++);
-				((unsigned long *)&atpdev->sp[m][0])[k] = atp_readl_base(atpdev, 0x38);
+				((u32 *)&atpdev->sp[m][0])[k] = atp_readl_base(atpdev, 0x38);
 			}
 			n += 8;
 		}

So this is untested ? If so please make it very clear in the commit message because the kernel is IMHO getting too full of polished, neat, recently modified, never tested, never used code.

I agree it would be better if the driver was simply deleted. I've not even seen an ATP870 bug report in years.

Alan

Maybe because it worked. Although the code was horrible. I've done some big changes to this driver recently (tested, of course). I can't test this patch as I don't have ATP885 card, only ATP870.

Ondrej Zary

Everyone knows I didn't test it but it's an obvious one line fix for memory corruption. If no one uses the code, at least this is harmless and silences a static checker warning.

In olden times we used to say, "Oh this bounds checking is crap but it's root only so let's leave it alone." But these days we just fix it. It's easier to just fix everything instead of trying to decide which bugs are critical.

regards,
dan carpenter

Unfortunately it's all too easy to look down 50 commit messages to an apaprently active file all "fixing small bugs" or "correcting indenting" without realising that every single one of them should have been tagged

"[UNTESTED]: "

so that anyone looking at the code can see immediately its historical hazardous waste.

Alan

We should add a tag to indicate that we are sending a patch for a crappy driver.

IMHO-this-driver-is-garbage: Your Name <email>

If it got 10 votes of no confidence it would be moved to staging and then deleted.

Anyway, realistically, let's just apply this fix. It's tempting to think we could delete all atp885 related code, but maybe people are still using it with 32 bit kernels. Or someone could delete it, but I'm not brave enough to be the one to do it.

regards,
dan carpenter

Forgive my ignorance, but what is the exact procedure? For example, the following file: drivers/pcmcia/vrc4173_cardu.c contains the following
code: INIT_WORK(&socket->tq_work, cardu_bh, socket);. The last time INIT_WORK took three arguments was Linux 2.6.19, so I think no one has been compiling this code recently. There would be the .c file and the associated .h file to move to staging, but it's less clear to me eg what to do with the Kconfig entry and the Makefile entry. And is there anything else to take into account?

thanks,
julia

You should just delete that code along with the Kconfig and Makefile entries. Don't bother moving it to staging. The move to staging is supposed to give people one last chance to yell if they absolutely need the code. No one has compiled this code for years so no one will miss it.

regards,
dan carpenter

And for stuff which might be worth saving (eg something that looks rather broken but has possibly got users) the driver goes into staging in its own directory and the Makefile and Kconfig entry for it move into the staging directory with the hope that someone screams and maintains it.

Alan

Ondrej, please review!

Martin K. Petersen	Oracle Linux Engineering

Dan,

Applied to 4.17/scsi-queue. Thanks!

Martin K. Petersen	Oracle Linux Engineering