linux-kernel - Re: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87prtw2413.fsf@saeurebad.de>
Date:	Sat, 15 Mar 2008 02:02:16 +0100
From:	Johannes Weiner <hannes@...urebad.de>
To:	Jesper Juhl <jesper.juhl@...il.com>
Cc:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
	Gadi Oxman <gadio@...vision.net.il>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline()

Hi Jesper,

Jesper Juhl <jesper.juhl@...il.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@...urebad.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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/