lkml.org 
[lkml]   [2008]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline()
Date

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


\
 
 \ /
  Last update: 2008-03-16 17:47    [W:0.113 / U:4.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site