Messages in this thread |  | | From | Praveen Srinivasan <> | Subject | Re: [PATCH] fsm.c - null ptr fixes for 2.4.4 | Date | Thu, 24 May 2001 22:26:34 -0700 |
| |
We tried to minimize the amount of changes we made; your patch is far more extensive. As this is not our code, we felt it would be a bad idea to make changes to the underlying structure.
Praveen Srinivasan and Frederick Akalin
Kai Germaschewski wrote:
> On Thu, 24 May 2001, Praveen Srinivasan wrote: > >> Using the Stanford checker, we searched for null-pointer bugs in the >> linux kernel code. This patch fixes numerous unchecked pointers in the >> ISDN hisax card driver (fsm.c). > > Is one numerous? Anyway, thanks for you effort. Your fix is not > correct though, because it replaces the bug with another one. > > In case the allocation fails, the current code will oops directly, so it's > quite easy to track down what went wrong. After applying your patch, the > code will still oops, but at a later time, when one of the not correctly > initialized state machines is actually used, so the problem is harder to > track. > > The correct way to fix the problem is something along the lines of the > appended patch. You need to notify the caller of the allocation failure > and handle it correctly. > > Thanks for your work, I'll take care of submitting the right fix to Linus. > > --Kai > > Index: callc.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/callc.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 callc.c > --- callc.c 2001/04/23 22:51:03 1.1.1.2 > +++ callc.c 2001/05/24 16:04:16 > @@ -850,14 +850,14 @@ > > #define FNCOUNT (sizeof(fnlist)/sizeof(struct FsmNode)) > > -void __init > +int __init > CallcNew(void) > { > callcfsm.state_count = STATE_COUNT; > callcfsm.event_count = EVENT_COUNT; > callcfsm.strEvent = strEvent; > callcfsm.strState = strState; > - FsmNew(&callcfsm, fnlist, FNCOUNT); > + return FsmNew(&callcfsm, fnlist, FNCOUNT); > } > > void > Index: config.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/config.c,v > retrieving revision 1.1.1.4.32.1 > diff -u -r1.1.1.4.32.1 config.c > --- config.c 2001/05/24 07:52:56 1.1.1.4.32.1 > +++ config.c 2001/05/24 16:17:00 > @@ -1332,18 +1332,28 @@ > > static int __init HiSax_init(void) > { > - int i; > + int i, retval; > #ifdef MODULE > int j; > int nzproto = 0; > #endif > > HiSaxVersion(); > - CallcNew(); > - Isdnl3New(); > - Isdnl2New(); > - TeiNew(); > - Isdnl1New(); > + retval = CallcNew(); > + if (retval) > + goto out; > + retval = Isdnl3New(); > + if (retval) > + goto out_callc; > + retval = Isdnl2New(); > + if (retval) > + goto out_isdnl3; > + retval = TeiNew(); > + if (retval) > + goto out_isdnl2; > + retval = Isdnl1New(); > + if (retval) > + goto out_tei; > > #ifdef MODULE > if (!type[0]) { > @@ -1490,17 +1500,26 @@ > printk(KERN_DEBUG "HiSax: Total %d card%s defined\n", > nrcards, (nrcards > 1) ? "s" : ""); > > - if (HiSax_inithardware(NULL)) { > - /* Install only, if at least one card found */ > - return (0); > - } else { > - Isdnl1Free(); > - TeiFree(); > - Isdnl2Free(); > - Isdnl3Free(); > - CallcFree(); > - return -EIO; > + /* Install only, if at least one card found */ > + if (!HiSax_inithardware(NULL)) { > + retval = -EIO; > + goto out_isdnl1; > } > + > + return 0; > + > + out_isdnl1: > + Isdnl1Free(); > + out_tei: > + TeiFree(); > + out_isdnl2: > + Isdnl2Free(); > + out_isdnl3: > + Isdnl3Free(); > + out_callc: > + CallcFree(); > + out: > + return retval; > } > > static void __exit HiSax_exit(void) > Index: fsm.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/fsm.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 fsm.c > --- fsm.c 2001/04/23 22:51:00 1.1.1.2 > +++ fsm.c 2001/05/24 16:03:22 > @@ -15,13 +15,16 @@ > > #define FSM_TIMER_DEBUG 0 > > -void __init > +int __init > FsmNew(struct Fsm *fsm, struct FsmNode *fnlist, int fncount) > { > int i; > > fsm->jumpmatrix = (FSMFNPTR *) > kmalloc(sizeof (FSMFNPTR) * fsm->state_count * fsm->event_count, > GFP_KERNEL); > + if (!fsm->jumpmatrix) > + return -ENOMEM; > + > memset(fsm->jumpmatrix, 0, sizeof (FSMFNPTR) * fsm->state_count * > fsm->event_count); > > for (i = 0; i < fncount; i++) > @@ -32,6 +35,7 @@ > } else > fsm->jumpmatrix[fsm->state_count * fnlist[i].event + > fnlist[i].state] = (FSMFNPTR) fnlist[i].routine; > + return 0; > } > > void > Index: hisax.h > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/hisax.h,v > retrieving revision 1.1.1.4 > diff -u -r1.1.1.4 hisax.h > --- hisax.h 2001/04/24 00:23:56 1.1.1.4 > +++ hisax.h 2001/05/24 16:09:43 > @@ -1304,7 +1304,7 @@ > int getcallref(u_char * p); > int newcallref(void); > > -void FsmNew(struct Fsm *fsm, struct FsmNode *fnlist, int fncount); > +int FsmNew(struct Fsm *fsm, struct FsmNode *fnlist, int fncount); > void FsmFree(struct Fsm *fsm); > int FsmEvent(struct FsmInst *fi, int event, void *arg); > void FsmChangeState(struct FsmInst *fi, int newstate); > @@ -1335,19 +1335,19 @@ > > int ll_run(struct IsdnCardState *cs, int addfeatures); > void ll_stop(struct IsdnCardState *cs); > -void CallcNew(void); > +int CallcNew(void); > void CallcFree(void); > int CallcNewChan(struct IsdnCardState *cs); > void CallcFreeChan(struct IsdnCardState *cs); > -void Isdnl1New(void); > +int Isdnl1New(void); > void Isdnl1Free(void); > -void Isdnl2New(void); > +int Isdnl2New(void); > void Isdnl2Free(void); > -void Isdnl3New(void); > +int Isdnl3New(void); > void Isdnl3Free(void); > void init_tei(struct IsdnCardState *cs, int protocol); > void release_tei(struct IsdnCardState *cs); > char *HiSax_getrev(const char *revision); > -void TeiNew(void); > +int TeiNew(void); > void TeiFree(void); > int certification_check(int output); > Index: isdnl1.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/isdnl1.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 isdnl1.c > --- isdnl1.c 2001/04/23 22:51:01 1.1.1.2 > +++ isdnl1.c 2001/05/24 16:06:14 > @@ -736,26 +736,41 @@ > > #define L1B_FN_COUNT (sizeof(L1BFnList)/sizeof(struct FsmNode)) > > -void __init > +int __init > Isdnl1New(void) > { > -#ifdef HISAX_UINTERFACE > - l1fsm_u.state_count = L1U_STATE_COUNT; > - l1fsm_u.event_count = L1_EVENT_COUNT; > - l1fsm_u.strEvent = strL1Event; > - l1fsm_u.strState = strL1UState; > - FsmNew(&l1fsm_u, L1UFnList, L1U_FN_COUNT); > -#endif > + int retval; > + > l1fsm_s.state_count = L1S_STATE_COUNT; > l1fsm_s.event_count = L1_EVENT_COUNT; > l1fsm_s.strEvent = strL1Event; > l1fsm_s.strState = strL1SState; > - FsmNew(&l1fsm_s, L1SFnList, L1S_FN_COUNT); > + retval = FsmNew(&l1fsm_s, L1SFnList, L1S_FN_COUNT); > + if (retval) > + return retval; > + > l1fsm_b.state_count = L1B_STATE_COUNT; > l1fsm_b.event_count = L1_EVENT_COUNT; > l1fsm_b.strEvent = strL1Event; > l1fsm_b.strState = strL1BState; > - FsmNew(&l1fsm_b, L1BFnList, L1B_FN_COUNT); > + retval = FsmNew(&l1fsm_b, L1BFnList, L1B_FN_COUNT); > + if (retval) { > + FsmFree(&l1fsm_s); > + return retval; > + } > +#ifdef HISAX_UINTERFACE > + l1fsm_u.state_count = L1U_STATE_COUNT; > + l1fsm_u.event_count = L1_EVENT_COUNT; > + l1fsm_u.strEvent = strL1Event; > + l1fsm_u.strState = strL1UState; > + retval = FsmNew(&l1fsm_u, L1UFnList, L1U_FN_COUNT); > + if (retval) { > + FsmFree(&l1fsm_s); > + FsmFree(&l1fsm_b); > + return retval; > + } > +#endif > + return 0; > } > > void Isdnl1Free(void) > Index: isdnl2.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/isdnl2.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 isdnl2.c > --- isdnl2.c 2001/04/23 22:51:01 1.1.1.2 > +++ isdnl2.c 2001/05/24 16:06:36 > @@ -1831,14 +1831,14 @@ > { > } > > -void __init > +int __init > Isdnl2New(void) > { > l2fsm.state_count = L2_STATE_COUNT; > l2fsm.event_count = L2_EVENT_COUNT; > l2fsm.strEvent = strL2Event; > l2fsm.strState = strL2State; > - FsmNew(&l2fsm, L2FnList, L2_FN_COUNT); > + return FsmNew(&l2fsm, L2FnList, L2_FN_COUNT); > } > > void > Index: isdnl3.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/isdnl3.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 isdnl3.c > --- isdnl3.c 2001/04/23 22:51:01 1.1.1.2 > +++ isdnl3.c 2001/05/24 16:07:16 > @@ -591,14 +591,14 @@ > } > } > > -void __init > +int __init > Isdnl3New(void) > { > l3fsm.state_count = L3_STATE_COUNT; > l3fsm.event_count = L3_EVENT_COUNT; > l3fsm.strEvent = strL3Event; > l3fsm.strState = strL3State; > - FsmNew(&l3fsm, L3FnList, L3_FN_COUNT); > + return FsmNew(&l3fsm, L3FnList, L3_FN_COUNT); > } > > void > Index: tei.c > =================================================================== > RCS file: /scratch/kai/cvsroot/linux_2_4/drivers/isdn/hisax/tei.c,v > retrieving revision 1.1.1.2 > diff -u -r1.1.1.2 tei.c > --- tei.c 2001/04/23 22:51:00 1.1.1.2 > +++ tei.c 2001/05/24 16:07:11 > @@ -446,14 +446,14 @@ > > #define TEI_FN_COUNT (sizeof(TeiFnList)/sizeof(struct FsmNode)) > > -void __init > +int __init > TeiNew(void) > { > teifsm.state_count = TEI_STATE_COUNT; > teifsm.event_count = TEI_EVENT_COUNT; > teifsm.strEvent = strTeiEvent; > teifsm.strState = strTeiState; > - FsmNew(&teifsm, TeiFnList, TEI_FN_COUNT); > + return FsmNew(&teifsm, TeiFnList, TEI_FN_COUNT); > } > > void > > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |