Messages in this thread Patch in this message | | | From | Johannes Weiner <> | Subject | Re: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline() | Date | Sat, 15 Mar 2008 02:02:16 +0100 |
| |
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?
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; rq->rq_disk = tape->disk; rq->buffer = NULL;
| |