Re: memory corruption/panic solved ("FAILURE - ATAPI_IDENTIFY no interrupt")

Nate Lawson wrote:
> I've tracked down the source of the memory corruption in -current that
> results when booting with various CD and DVD drives (especially the ones
> that come with Thinkpads including T23, R32, T41, etc.)  The panic is
> obvious when running with INVARIANTS ("memory modified after free") but
> not so obvious in other configurations.  For instance, without
> INVARIANTS, part of the rt_info structure is corrupted on my wireless
> card, resulting in a panic during ifconfig on boot.  This is likely the
> source of other problems, including phk's ACPI panic (again, only
> triggered when booting with the CD drive in the bay.)

OK, first thanks for digging into this!

> The root problem is that ata_timeout() fires and calls ata_pio_read()
> which overwrites 512 bytes random memory.  There are actually two bugs 
> here that overwrite memory.  The code path is as follows:
> 
> 1. ata runs an IDENTIFY command on each drive.  It reaches this stack:
> 
> ata_getparam()
> ata_identify_devices()
> ata_boot_attach()
> 
> 2. ata_getparam() allocates a request and runs it:
> ata_alloc_request()
> loop on retries (2 max)
> fill out an immediate read request for 512 bytes (DEV_BSIZE)
> *** Bug 1: transfersize is 512 bytes but sizeof(struct ata_request) is 
> much less (~80 bytes).
> ata_queue_request() starts the request and arms a timeout

Its correct to allocate via ata_alloc_request, the data is *not* put 
into the request, but into another memory area (atadev->param) so this 
is not a bug.

> 3. Completion for first request results in FAILURE - no interrupt. 
> ata_completed() calls ata_generic_interrupt() which calls ata_pio_read()
> *** Bug 1: here's where the 512 - sizeof(struct ata_request) bytes 
> following "request" are overwritten.

See above, the data hits the malloc'd buffer (atadev->param) *not* the 
request.

> . The completion code also updates request->donecount (an 
> upward-counting residual) from 0 to 512 after the request has been 
> completed.
> 
> 4. ata_getparam runs through the loop again and starts another identify
> *** Bug 2: donecount is now 512 so ata_pio_read() will write on 
> request->data + 512 with 512 bytes.  Whatever follows request->data + 
> 512 is overwritten.

Thats the real bug, donecount should be reset on retry..

> _at__at_ -561,11 +561,12 _at__at_
>  		request->data = (caddr_t)atadev->param;
>  		request->bytecount = sizeof(struct ata_params);
>  		request->transfersize = DEV_BSIZE;
> +		request->donecount = 0;
>  		ata_queue_request(request);
>  		if (!(error = request->result))
>  		    break;
>  	    }

This part should be all thats needed IMHO...

-Søren