Re: [PATCH v1 1/2] ata: sata_dwc_460ex: Fix crash due to OOB write

[Christian Lamparter <chunkeey@gmail.com>]

On 18/03/2022 15:16, Andy Shevchenko wrote:
> On Fri, Mar 18, 2022 at 10:03:11AM +0100, Christian Lamparter wrote:
>> the driver uses libata's "tag" values from in various arrays.
>> Since the mentioned patch bumped the ATA_TAG_INTERNAL to 32,
>> the value of the SATA_DWC_QCMD_MAX needs to be bumped to 33.
>>
>> Otherwise ATA_TAG_INTERNAL cause a crash like this:
>>
>> | BUG: Kernel NULL pointer dereference at 0x00000000
>> | Faulting instruction address: 0xc03ed4b8
>> | Oops: Kernel access of bad area, sig: 11 [#1]
>> | BE PAGE_SIZE=4K PowerPC 44x Platform
>> | CPU: 0 PID: 362 Comm: scsi_eh_1 Not tainted 5.4.163 #0
>> | NIP:  c03ed4b8 LR: c03d27e8 CTR: c03ed36c
>> | REGS: cfa59950 TRAP: 0300   Not tainted  (5.4.163)
>> | MSR:  00021000 <CE,ME>  CR: 42000222  XER: 00000000
>> | DEAR: 00000000 ESR: 00000000
>> | GPR00: c03d27e8 cfa59a08 cfa55fe0 00000000 0fa46bc0 [...]
>> | [..]
>> | NIP [c03ed4b8] sata_dwc_qc_issue+0x14c/0x254
>> | LR [c03d27e8] ata_qc_issue+0x1c8/0x2dc
>> | Call Trace:
>> | [cfa59a08] [c003f4e0] __cancel_work_timer+0x124/0x194 (unreliable)
>> | [cfa59a78] [c03d27e8] ata_qc_issue+0x1c8/0x2dc
>> | [cfa59a98] [c03d2b3c] ata_exec_internal_sg+0x240/0x524
>> | [cfa59b08] [c03d2e98] ata_exec_internal+0x78/0xe0
>> | [cfa59b58] [c03d30fc] ata_read_log_page.part.38+0x1dc/0x204
>> | [cfa59bc8] [c03d324c] ata_identify_page_supported+0x68/0x130
>> | [...]
>>
>> this is because sata_dwc_dma_xfer_complete() NULLs the
>> dma_pending's next neighbour "chan" (a *dma_chan struct) in
>> this '32' case right here (line ~735):
>>> hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_NONE;
>>
>> Then the next time, a dma gets issued; dma_dwc_xfer_setup() passes
>> the NULL'd hsdevp->chan to the dmaengine_slave_config() which then
>> causes the crash.
> 
> ...
> 
>> ticerex said when I've asked him about his real name+email for the patches:
>> "Please use my forum nick."
>> <https://forum.openwrt.org/t/my-book-live-duo-reboot-loop/122464/14>
>> (I know checkpatch.pl complains about that. But what can you do?)
> 
> I think Reported-by: is fine to have any kind of reference to the reporter.
> I can consider it false positive.
> 
> ...

K, I've reported this back to the reporter ;).

Documentation/process/maintainer-tip.rst and process/5.Posting.rst
provided some hints:

"Please note that if the bug was reported in private, then ask for
permission first before using the Reported-by tag."

and maintainer-tip.rst, the format should be:
``Reported-by: ``Reporter <reporter@mail>``

(My goal here is to get "a fix" merged, so conformance is key. ;-))

>> -#define SATA_DWC_QCMD_MAX	32
>> +#define SATA_DWC_QCMD_MAX	33
> 
> Can't we use
> 
> #define SATA_DWC_QCMD_MAX	(ATA_TAG_INTERNAL + 1)
> 

I've looked around a bit.

include/linux/libata.h itself has the following related definitions:

| enum {
|        ATA_MAX_QUEUE           = 32,
|        ATA_TAG_INTERNAL        = ATA_MAX_QUEUE,
| [..]

| struct ata_port {
|	[...]
| 	struct ata_queued_cmd   qcmd[ATA_MAX_QUEUE + 1];
|
| }

ATA_MAX_QUEUE seems to be the "size" value. Whereas ATA_TAG_INTERNAL
is used as (tag == ATA_TAG_INTERNAL) more often.


I came up with a viable compromise:

Would it be "OK" to define a "new" ATA_MAX_TAG?

This could be either set ATA_MAX_TAG = ATA_MAX_QUEUE + 1

or let it be assigned automatically, if it's placed after ATA_TAG_INTERNAL
in the libata.h's enum like this: (I prefer this, but it being "33" is not
obvious if you don't dabble in C all the time)

| enum {
|	[...]
|       ATA_MAX_QUEUE           = 32,
|       ATA_TAG_INTERNAL        = ATA_MAX_QUEUE,
|	/*
|	 * ATA_MAX_TAG accounts for ATA_MAX_QUEUE TAGs + 1 special TAG/slot
|	 * for ATA_TAG_INTERNAL (libata's internal commands/DMA management).
|	 * This needs to be it in this location.
|	 */
|	ATA_MAX_TAG,
|       [...]

This ATA_MAX_TAG could then be used for ata_port's qcmd (from above) and
sata_dwc_460ex's SATA_DWC_QCMD_MAX.

For good measure:

BUILD_BUG_ON(ATA_TAG_INTERNAL >= ATA_MAX_TAG);

could be added into libata.h's __ata_qc_from_tag().
It access the qcmd array, so anyone using libata's accessors will catch
future updates.

Is this fine or getting to close to being overbuild?

Thank you both for your insights,
Christian