Messages in this thread | | | From | Bartlomiej Zolnierkiewicz <> | Subject | Re: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline() | Date | Sun, 16 Mar 2008 17:58:22 +0100 |
| |
Hi,
On Saturday 15 March 2008, Johannes Weiner wrote: > Hi Jesper, > > Jesper Juhl <jesper.juhl@gmail.com> writes: > > > If a NULL 'new_last_stage' is passed to idetape_abort_pipeline() then > > we'll dereference a NULL pointer and go *boom*. > > The function does test for a null pointer, unfortunately it only does it > > after having already dereferenced it. > > Did you hit an oops because of this? > > > @@ -814,11 +814,14 @@ static void idetape_abort_pipeline(ide_drive_t *drive, > > idetape_stage_t *new_last_stage) > > { > > idetape_tape_t *tape = drive->driver_data; > > - idetape_stage_t *stage = new_last_stage->next; > > + idetape_stage_t *stage = NULL; > > idetape_stage_t *nstage; > > > > debug_log(DBG_PROCS, "%s: Enter %s\n", tape->name, __func__); > > > > + if (new_last_stage) > > + stage = new_last_stage->next; > > + > > while (stage) { > > nstage = stage->next; > > idetape_kfree_stage(tape, stage); > > ] --tape->nr_stages; > ] --tape->nr_pending_stages; > ] stage = nstage; > ] } > ] if (new_last_stage) > ] new_last_stage->next = NULL; > > ... because if not, and new_last_stage will never be NULL at all in this > function, the check here could be removed instead of adding another one. > Or perhaps a BUG_ON(!stage) in idetape_end_request() already? > > Bartlomiej, please have a look at the following patch. Should all of > these hand-checks in the file be replaced by BUG_ON()s? Or be removed > completely?
I think that they should be removed completely.
> Hannes > > -- > > Turn possible NULL-pointer dereference in idetape_active_next_stage() > into an explicit bug and remove the warn-only checking for it. > > Signed-off-by: Johannes Weiner <hannes@saeurebad.de> > > --- > The explicit checking of @stage indicates that someone was expecting > that it could be NULL here. Could someone with real understanding of > the code check if the condition is realistic? > > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c > index 43e0e05..b63f928 100644 > --- a/drivers/ide/ide-tape.c > +++ b/drivers/ide/ide-tape.c > @@ -724,17 +724,15 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense) > > static void idetape_activate_next_stage(ide_drive_t *drive) > { > + struct request *rq; > idetape_tape_t *tape = drive->driver_data; > idetape_stage_t *stage = tape->next_stage; > - struct request *rq = &stage->rq; > > debug_log(DBG_PROCS, "Enter %s\n", __func__); > > - if (stage == NULL) { > - printk(KERN_ERR "ide-tape: bug: Trying to activate a non" > - " existing stage\n"); > - return; > - } > + BUG_ON(!stage); > + > + rq = &stage->rq;
[ stage->rq will OOPS anyway in case of bug so no need for BUG_ON() ]
> rq->rq_disk = tape->disk; > rq->buffer = NULL;
Please recast the patch and resumbit.
Thanks, Bart
| |