lkml.org 
[lkml]   [2008]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH] ide-tape: Avoid potential null pointer dereference in idetape_abort_pipeline()
Date
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;


\
 
 \ /
  Last update: 2008-03-15 02:07    [W:0.046 / U:10.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site