Re: [PATCH] ide-cd: prevent null pointer deref via cdrom_newpc_intr

From: Rainer Weikusat
Date: Thu Jun 18 2009 - 12:19:04 EST

Borislav Petkov <petkovbb@xxxxxxxxxxxxxx> writes:
> On Thu, Jun 18, 2009 at 4:52 PM, Rainer Weikusat<rweikusat@xxxxxxxxxxx> wrote:
>> Borislav Petkov <petkovbb@xxxxxxxxxxxxxx> writes:


>>> so this request is completed as a whole and the rq
>>> freed."
>> Technically, this is not quite correct (assuming I haven't overlooked
>> something), because ide_cd_queue_pc still has a reference to the rq.
> That doesn't matter because the OOPS happens after the command has been
> issued and _before_ ide_cd_queue_pc() gets to access the rq ptr
> again.

Yes. Because the pointer I already mentioned has been reset. But the
request itself is still alive.


> ide_complete_rq simply does
> blk_end_request
> |->blk_end_bidi_request
> |->blk_finish_request
> after checking the rq->bio pointer through blk_update_bidi_request() and
> if the prior is NULL it does __blk_put_request in blk_finish_request and
> this is where the thing vanishes.
> The second time ide_complete_rq() comes to run at the end of the IRQ
> handler the rq is already 0xdeadbeef :).

Not quite. Below is the blk_execute_rq-function:

int blk_execute_rq(struct request_queue *q, struct gendisk *bd_disk,
struct request *rq, int at_head)
int err = 0;

* we need an extra reference to the request, so we can look at
* it after io completion

if (!rq->sense) {
memset(sense, 0, sizeof(sense));
rq->sense = sense;
rq->sense_len = 0;

rq->end_io_data = &wait;
blk_execute_rq_nowait(q, bd_disk, rq, at_head, blk_end_sync_rq);

if (rq->errors)
err = -EIO;

return err;

and the refcount is incremented at the start of that 'so we can look
at it after io-completion', which means 'after the code below has

static void blk_end_sync_rq(struct request *rq, int error)
struct completion *waiting = rq->end_io_data;

rq->end_io_data = NULL;
__blk_put_request(rq->q, rq);

* complete last, if this is a stack request the process (and thus
* the rq pointer) could be invalid right after this complete()

which puts the rq once, decrementing the refcount by
one. Both blk_execute_rq and ide_cd_queue_pc inspect the rq after it
has been completed and the latter puts it again. While inspecting a
'freed' data structure would probably be harmless, freeing it twice
would be quite of a problem :-). I have spent something like 18 hours
of my life to determine what was going on here, starting from 'I have
never seen any of this again' and came across a bit of it in the

But I am actually busy doing 'something completely different' with 2.4
for my job ...
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at