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

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

[Jesper Juhl]

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.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 ide-tape.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 43e0e05..943290c 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -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);

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

[Johannes Weiner]

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;

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

[Jesper Juhl]

On 15/03/2008, Johannes Weiner <hannes@saeurebad.de> 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?
>

No, I did not.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

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@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