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:
--- linux/drivers/isdn/hisax/config.c-orig Tue Dec 4 02:09:59 2001
+++ linux/drivers/isdn/hisax/config.c Tue Dec 4 17:30:29 2001
@@ -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:
--- linux/drivers/isdn/Config.in-orig Sat Dec 8 00:35:27 2001
+++ linux/drivers/isdn/Config.in Sat Dec 8 00:35:15 2001
@@ -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
--- linux/drivers/isdn/hisax/Makefile-orig Sat Dec 8 00:24:43 2001
+++ linux/drivers/isdn/hisax/Makefile Sat Dec 8 00:25:39 2001
@@ -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
--- linux/drivers/isdn/hisax/hisax.h-orig Sat Dec 8 00:24:20 2001
+++ linux/drivers/isdn/hisax/hisax.h Sat Dec 8 00:40:49 2001
@@ -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
--- linux/drivers/isdn/hisax/config.c-orig Sat Dec 8 00:24:26 2001
+++ linux/drivers/isdn/hisax/config.c Sat Dec 8 00:33:57 2001
@@ -336,17 +336,8 @@
NULL, \
}
-#define EMPTY_CARD {0, DEFAULT_PROTO, {0, 0, 0, 0}, NULL}
-
struct IsdnCard cards[HISAX_MAX_CARDS] = {
FIRST_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
- EMPTY_CARD,
};
#define HISAX_IDSIZE (HISAX_MAX_CARDS*8)
@@ -454,6 +445,7 @@
i = 0;
j = 1;
while (argc && (i < HISAX_MAX_CARDS)) {
+ cards[i].protocol = DEFAULT_PROTO;
if (argc) {
cards[i].typ = ints[j];
j++;
@@ -1405,6 +1397,8 @@
cards[j].protocol = protocol[i];
nzproto++;
}
+ else
+ cards[j].protocol = DEFAULT_PROTO;
switch (type[i]) {
case ISDN_CTYPE_16_0:
cards[j].para[0] = irq[i];
@@ -1481,15 +1475,22 @@
} else {
/* QUADRO is a 4 BRI card */
cards[j++].para[0] = 1;
- cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
- cards[j].protocol = protocol[i];
- cards[j++].para[0] = 2;
- cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
- cards[j].protocol = protocol[i];
- cards[j++].para[0] = 3;
- cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
- cards[j].protocol = protocol[i];
- cards[j].para[0] = 4;
+ /* we need to check if further cards can be added */
+ if (j < HISAX_MAX_CARDS) {
+ cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
+ cards[j].protocol = protocol[i];
+ cards[j++].para[0] = 2;
+ }
+ if (j < HISAX_MAX_CARDS) {
+ cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
+ cards[j].protocol = protocol[i];
+ cards[j++].para[0] = 3;
+ }
+ if (j < HISAX_MAX_CARDS) {
+ cards[j].typ = ISDN_CTYPE_SCT_QUADRO;
+ cards[j].protocol = protocol[i];
+ cards[j].para[0] = 4;
+ }
}
break;
}
@@ -1563,6 +1564,8 @@
if (protocol[i]) {
cards[i].protocol = protocol[i];
}
+ else
+ cards[i].protocol = DEFAULT_PROTO;
}
cards[0].para[0] = pcm_irq;
cards[0].para[1] = (int) pcm_iob;
@@ -1603,6 +1606,8 @@
if (protocol[i]) {
cards[i].protocol = protocol[i];
}
+ else
+ cards[i].protocol = DEFAULT_PROTO;
}
cards[0].para[0] = pcm_irq;
cards[0].para[1] = (int) pcm_iob;
@@ -1643,6 +1648,8 @@
if (protocol[i]) {
cards[i].protocol = protocol[i];
}
+ else
+ cards[i].protocol = DEFAULT_PROTO;
}
cards[0].para[0] = pcm_irq;
cards[0].para[1] = (int) pcm_iob;
@@ -1683,6 +1690,8 @@
if (protocol[i]) {
cards[i].protocol = protocol[i];
}
+ else
+ cards[i].protocol = DEFAULT_PROTO;
}
cards[0].para[0] = pcm_irq;
cards[0].para[1] = (int) pcm_iob;
On Sat, 8 Dec 2001, Stephan von Krawczynski wrote:
> attached is the second patch for ISDN-Driver HiSax for kernel-inclusion that does the
> following:
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)
> +/* string magic */
> +#define h1(s) h2(s)
> +#define h2(s) #s
> +#define PARM_PARA "1-"h1(HISAX_MAX_CARDS)"i"
> +
you should use __MODULE_STRING (from linux/module.h)
> --- linux/drivers/isdn/hisax/hisax.h-orig Sat Dec 8 00:24:20 2001
> +++ linux/drivers/isdn/hisax/hisax.h Sat Dec 8 00:40:49 2001
> @@ -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
Why is that necessary?
> @@ -1563,6 +1564,8 @@
> if (protocol[i]) {
> cards[i].protocol = protocol[i];
> }
> + else
> + cards[i].protocol = DEFAULT_PROTO;
> }
> cards[0].para[0] = pcm_irq;
> cards[0].para[1] = (int) pcm_iob;
nitpicking:
This should be
} else
cards[i]...
Actually, I'd prefer
} else {
cards[i]...
}
I'll fix up these small bits, commit it to our CVS and take care of
submitting the patch.
Thanks again for your work.
--Kai
> On Sat, 8 Dec 2001, Stephan von Krawczynski wrote:
>
> > attached is the second patch for ISDN-Driver HiSax for
kernel-inclusion that does the
> > following:
>
> 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)
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 :-)
> This makes the procedure take a bit longer, but things like missing
> semicolons get caught in the process ;-)
Yes, I declare myself guilty. It's a nice example how a real simlpe
patch woes.
> 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).
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.
> Some comments:
>
> (original patch)
>
> > +/* string magic */
> > +#define h1(s) h2(s)
> > +#define h2(s) #s
> > +#define PARM_PARA "1-"h1(HISAX_MAX_CARDS)"i"
> > +
> you should use __MODULE_STRING (from linux/module.h)
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.
> > --- linux/drivers/isdn/hisax/hisax.h-orig Sat Dec 8 00:24:20 2001
> > +++ linux/drivers/isdn/hisax/hisax.h Sat Dec 8 00:40:49 2001
> > @@ -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
>
> Why is that necessary?
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.
> > @@ -1563,6 +1564,8 @@
> > if (protocol[i]) {
> > cards[i].protocol = protocol[i];
> > }
> > + else
> > + cards[i].protocol = DEFAULT_PROTO;
> > }
> > cards[0].para[0] = pcm_irq;
> > cards[0].para[1] = (int) pcm_iob;
>
> nitpicking:
>
> This should be
> } else
> cards[i]...
>
> Actually, I'd prefer
>
> } else {
> cards[i]...
> }
Feel free. I in fact thought about the other way round, but that's
only a personal viewing and reading issue.
> I'll fix up these small bits, commit it to our CVS and take care of
> submitting the patch.
>
> Thanks again for your work.
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