hisax fix MAX_CARD setup and potential buffer overflow

Hello Karsten,
Hello Kai,

attached is a patch for ISDN-Driver HiSax for kernel-inclusion that does the following:

1) Fix usage of HISAX_MAX_CARDS definition so one can _really_ create more (or less) cards by simply editing HISAX_MAX_CARDS to an appropriate value. In the original code the idea was visible, only implementation was broken in this respect.

2) Fix a potential buffer overflow (strcpy) which can be triggered by adding too long IDs during insmod.

Thanks to Henning Schmiedehausen for support.

Hopefully coming soon:

Patch to edit HISAX_MAX_CARDS during make menuconfig (configurable for joe user)

Regards,
Stephan

PS: skraw back to the roots ;-)

hisax-max-patch:

@@ -338,7 +338,7 @@
 
 #define EMPTY_CARD	{0, DEFAULT_PROTO, {0, 0, 0, 0}, NULL}
 
-struct IsdnCard cards[] = {
+struct IsdnCard cards[HISAX_MAX_CARDS] = {
 	FIRST_CARD,
 	EMPTY_CARD,
 	EMPTY_CARD,

@@ -349,14 +349,15 @@
 	EMPTY_CARD,
 };
 
-static char HiSaxID[64] __devinitdata = { 0, };
+#define HISAX_IDSIZE (HISAX_MAX_CARDS*8)
+static char HiSaxID[HISAX_IDSIZE] __devinitdata = { 0, };
 
 char *HiSax_id __devinitdata = HiSaxID;
 #ifdef MODULE
 /* Variables for insmod */
-static int type[8] __devinitdata = { 0, };
-static int protocol[8] __devinitdata = { 0, };
-static int io[8] __devinitdata = { 0, };
+static int type[HISAX_MAX_CARDS] __devinitdata = { 0, };
+static int protocol[HISAX_MAX_CARDS] __devinitdata = { 0, };
+static int io[HISAX_MAX_CARDS] __devinitdata = { 0, };
 #undef IO0_IO1
 #ifdef CONFIG_HISAX_16_3
 #define IO0_IO1

@@ -366,25 +367,30 @@
 #define IO0_IO1
 #endif
 #ifdef IO0_IO1
-static int io0[8] __devinitdata = { 0, };
-static int io1[8] __devinitdata = { 0, };
+static int io0[HISAX_MAX_CARDS] __devinitdata = { 0, };
+static int io1[HISAX_MAX_CARDS] __devinitdata = { 0, };
 #endif
-static int irq[8] __devinitdata = { 0, };
-static int mem[8] __devinitdata = { 0, };
+static int irq[HISAX_MAX_CARDS] __devinitdata = { 0, };
+static int mem[HISAX_MAX_CARDS] __devinitdata = { 0, };
 static char *id __devinitdata = HiSaxID;
 
+/* string magic */
+#define h1(s) h2(s)
+#define h2(s) #s
+#define PARM_PARA "1-"h1(HISAX_MAX_CARDS)"i"
+
 MODULE_DESCRIPTION("ISDN4Linux: Driver for passive ISDN cards");
 MODULE_AUTHOR("Karsten Keil");
 MODULE_LICENSE("GPL");
-MODULE_PARM(type, "1-8i");
-MODULE_PARM(protocol, "1-8i");
-MODULE_PARM(io, "1-8i");
-MODULE_PARM(irq, "1-8i");
-MODULE_PARM(mem, "1-8i");
+MODULE_PARM(type, PARM_PARA);
+MODULE_PARM(protocol, PARM_PARA);
+MODULE_PARM(io, PARM_PARA);
+MODULE_PARM(irq, PARM_PARA);
+MODULE_PARM(mem, PARM_PARA);
 MODULE_PARM(id, "s");
 #ifdef IO0_IO1
-MODULE_PARM(io0, "1-8i");
-MODULE_PARM(io1, "1-8i");
+MODULE_PARM(io0, PARM_PARA);
+MODULE_PARM(io1, PARM_PARA);
 #endif
 #endif /* MODULE */
 

@@ -476,12 +482,14 @@
 		i++;
 	}
 	if (str && *str) {
-		strcpy(HiSaxID, str);
-		HiSax_id = HiSaxID;
-	} else {
+		if (strlen(str) < HISAX_IDSIZE)
+			strcpy(HiSaxID, str);
+		else
+			printk(KERN_WARNING "HiSax: ID too long!")
+	} else
 		strcpy(HiSaxID, "HiSax");
-		HiSax_id = HiSaxID;
-	}
+
+	HiSax_id = HiSaxID;
 	return 1;
 }
 

Hello Karsten,
Hello Kai,

attached is the second patch for ISDN-Driver HiSax for kernel-inclusion that does the following:

1) Make HISAX_MAX_CARDS user configurable during make menuconfig

2) Fix a potential trashing of data for the auto-add of SCITEL QUAD card. It did not check at all, if there was enough room to include further cards!

Patch is diffed to 2.4.17-pre6 (yes I am fast :-) and compiles ok.
As it is a bit bigger than originaly intended, can you please comment on it, Kai. I had to fix the init part, because you can now configure the MAX_CARDS-value _down_ to 1.

Thanks to Keith Owens for Makefile fiddling.

Regards,
Stephan

hisax-max-patch-2:

@@ -42,6 +42,7 @@
    fi
    bool '  HiSax Support for german 1TR6' CONFIG_HISAX_1TR6
    bool '  HiSax Support for US NI1' CONFIG_HISAX_NI1
+   int  '  Maximum number of cards supported by HiSax' CONFIG_HISAX_MAX_CARDS 8
    comment '  HiSax supported cards'
    bool '  Teles 16.0/8.0' CONFIG_HISAX_16_0
    bool '  Teles 16.3 or PNP or PCMCIA' CONFIG_HISAX_16_3

@@ -4,6 +4,10 @@
 
 O_TARGET	  := vmlinux-obj.o
 
+# Define maximum number of cards
+
+EXTRA_CFLAGS      += -DHISAX_MAX_CARDS=$(CONFIG_HISAX_MAX_CARDS)
+
 # Objects that export symbols.
 
 export-objs	  := config.o fsm.o hisax_isac.o

@@ -950,7 +950,9 @@
 #define  MON0_TX	4
 #define  MON1_TX	8
 
+#ifndef HISAX_MAX_CARDS
 #define	 HISAX_MAX_CARDS	8
+#endif
 
 #define  ISDN_CTYPE_16_0	1
 #define  ISDN_CTYPE_8_0		2

Please send patches to the maintainers (i.e. Karsten/me) first, not directly to Linus/Marcelo. Also, CC'ing lkml isn't really necessary, either. (Documentation/SubmittingPatches is worth reading)

This makes the procedure take a bit longer, but things like missing semicolons get caught in the process ;-)

So let's drop Linus/Marcelo/lkml from CC. I'll submit the patch eventually. (If it's already applied, that's fine, too)

Generally the patch looks good (still wondering if you've got that many cards in one machine).

Some comments:

(original patch)

		} else
			cards[i]...

		} else {
			cards[i]...
		}

kernel-inclusion that does the

Well, in fact my hope was, somebody from lkml would give it a quick look, because it again turned out the original author may not see minor problems :-)

Yes, I declare myself guilty. It's a nice example how a real simlpe patch woes.

many

Lots. :-)
Last event which made me think about this patch to submit (and not just hacking in every time I needed it), were 12. But to be honest, I made it work for less than 8 because I do think that 8 is a bit oversized for most people.

I love to in the future, now that I _know_ it. :-) Please convert it on your next submission to Marcelo & L. We don't wanna keep superfluous code.

Hm, good question. The thing is: I wanted to make 1000% sure it doesn't get lost. This should have been already in the first patch, where it was not really clear how to get the CONFIG definition. Additionally I feel uncomfortable with code being not complete in terms of symbols. You cannot grep through *.c/*.h to find missing/mis-spelled symbols then. I always lived with the idea that compile-time definitions _tune_ the code, but it should be complete even without. In fact it doesn't make a difference in the created binary.

Feel free. I in fact thought about the other way round, but that's only a personal viewing and reading issue.

You're welcome.

While reading config.c I came to the conclusion that this is pretty "organic" code :-) .
As in most ISDN drivers I read so far there is a whole lot of duplication inside. I had the strong urge to clean it up, but didn't want to submit a whole new file as a patch dealing with something completely different.
Perhaps I will in the future.

Regards,
Stephan