Re: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline()

[Bartlomiej Zolnierkiewicz]

Hi,

On Saturday 15 March 2008, Johannes Weiner wrote:
> Hi Jesper,
> 
> Jesper Juhl <jesper.juhl@xxxxxxxxx> 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@xxxxxxxxxxxx>
> 
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/