linux-kernel - [drivers/dma/ti] Question about `struct omap_desc`: misuse of __counted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAPbMC74cxADGEd2bc8wYA4iUwkP2f-Uywf=V1aMYs4iSiqh1sg@mail.gmail.com>
Date: Mon, 19 Aug 2024 11:53:23 -0700
From: Haoyu Li <lihaoyu499@...il.com>
To: Peter Ujfalusi <peter.ujfalusi@...il.com>, Vinod Koul <vkoul@...nel.org>
Cc: dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [drivers/dma/ti] Question about `struct omap_desc`: misuse of __counted_by

Dear Linux Developers for TEXAS INSTRUMENTS DMA DRIVERS,

We are curious about the use of `struct omap_desc`. Its definition is
at https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma/ti/omap-dma.c#L111.
```
struct omap_desc {
struct virt_dma_desc vd;
bool using_ll;
enum dma_transfer_direction dir;
dma_addr_t dev_addr;
bool polled;

int32_t fi; /* for OMAP_DMA_SYNC_PACKET / double indexing */
int16_t ei; /* for double indexing */
uint8_t es; /* CSDP_DATA_TYPE_xxx */
uint32_t ccr; /* CCR value */
uint16_t clnk_ctrl; /* CLNK_CTRL value */
uint16_t cicr; /* CICR value */
uint32_t csdp; /* CSDP value */

unsigned sglen;
struct omap_sg sg[] __counted_by(sglen);
};
```

Our question is: The `sg` member of `struct omap_desc` is annotated
with "__counted_by", which means the size of the array is indicated by
`sglen`. Only if we set `sglen` before accessing `sg[0]`, the flexible
member `item` can be properly bounds-checked at run-time when enabling
CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Or there will be a
warning from each array access that is prior to the initialization
because the number of elements is zero.

So we think relocating `d->sglen = 1` before accessing `d->sg[0]` is
needed in the following three positions:
- https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma/ti/omap-dma.c#L1192
- https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma/ti/omap-dma.c#L1264
- https://elixir.bootlin.com/linux/v6.10.6/source/drivers/dma/ti/omap-dma.c#L1319
Perhaps we can set `d->sglen = 1` right after `d = kzalloc(...)` operation.

Here is a fix example of a similar situation :
https://lore.kernel.org/stable/20240613113225.898955993@linuxfoundation.org/.

Please kindly correct us if we missed any key information. Looking
forward to your response!

Best,
Haoyu Li