[nvmewin] Potential BSOD if DataTransferLength < Allocation Length

[Mike Berhan]

I recently joined this reflector.  Advanced apologies if this has already
been covered.

I've been perusing the latest NVMe OFA source code.  The NVMe Win source
indicates a potential BSOD on a poorly formatted SPTI/SRB/CDB.  For the sake
of discussion, I'll limit this to the Report LUNs CDB but it's applicable to
a variety of other CDBs the driver handles.  Here is a sample SRB/CDB to
consider:

_SCSI_REQUEST_BLOCK
    <snip>
    ULONG DataTransferLength  00000100h
    <snip>
    PVOID DataBuffer          0000006C`801D0A50h
    <snip>
    UCHAR Cdb[16]             A0 00 00 00 00 00 00 00 10 00 00 00 00 00 00
00

The key items to consider are the DataTransferLength at 100h and the CDB
Allocation Length at 1000h.  This is likely a bug in the calling software as
the DataTransferLength should be greater than or equal to the CDB allocation
length.  In this example, this probably doesn't cause a problem as the
return data is likely less than 100h bytes anyway so the caller would never
see any failure.  However, when executed on the NVMe driver, several
problems can occur.

In reviewing the source code, the Report LUNs CDB gets routed to the
SntiTranslateReportLuns() function.  The code retrieves information about
the command as you see here:

    pResponseBuffer = (PUCHAR)GET_DATA_BUFFER(pSrb);
    allocLength = GET_REPORT_LUNS_ALLOC_LENGTH(pSrb);
    selectReport = GET_U8_FROM_CDB(pSrb, REPORT_LUNS_SELECT_REPORT_OFFSET);

>From our example above, the following values are extracted from the SRB/CDB:

    pResponseBuffer = 0000006C`801D0A50h
    allocLength = 1000h
    selectReport = 00h (i.e. RESTRICTED_LUNS_RETURNED)

The following code then gets executed:

    if ((selectReport != ALL_LUNS_RETURNED)            &&
        (selectReport != ALL_WELL_KNOWN_LUNS_RETURNED) &&
        (selectReport != RESTRICTED_LUNS_RETURNED)) {
        <snip>
    } else {
        <snip>
        memset(pResponseBuffer, 0, allocLength);   <<<--- BSOD!?!?!?
        <snip>
    }

Several potential problems are seen in walking through the source code.

Problem #1 - Driver overrunning data buffer

The miniport driver received a 100h byte buffer.  However, because the CDB
allocation length was set to 1000h, the miniport driver is going to overflow
the buffer and zero out 1000h bytes of data.  At best you're looking at a
BSOD.  At worst memory is corrupted.  It's also not checking for a NULL SRB
Data buffer pointer or the SRB data direction flags.

Problem #2 - Miniport writing to buffer when it shouldn't

A storage device should only alter the data buffer that has actual data.
For example, if I send down a request with a 256 byte buffer, but the device
only returns 16 bytes, the remainder of the buffer (i.e. the tail 240 bytes)
should not be altered.  The DataTransferLength field in the SRB would be
returned as 16 indicating that only 16 bytes were transferred.

In the sample cited above, the miniport driver is attempting to zero out the
entire data buffer passed in when it should only zero out the portion of the
data buffer that it will actually return data in (max DataTransferLength).
Not that this is a particularly critical issue but it is worth noting and
would make the driver more robust.

Mike