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 <[email protected]>
---
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);
Hi Jesper,
Jesper Juhl <[email protected]> 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 <[email protected]>
---
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;
On 15/03/2008, Johannes Weiner <[email protected]> wrote:
> Hi Jesper,
>
>
> Jesper Juhl <[email protected]> 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 <[email protected]>
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
Hi,
On Saturday 15 March 2008, Johannes Weiner wrote:
> Hi Jesper,
>
> Jesper Juhl <[email protected]> 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 <[email protected]>
>
> ---
> 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