| 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
| ||
|
Message-ID: <87v7lhwkf2.wl-tiwai@suse.de>
Date: Wed, 17 Sep 2025 10:30:25 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Luca Weiss" <luca.weiss@...rphone.com>
Cc: "Takashi Iwai" <tiwai@...e.de>,
"Arnd Bergmann" <arnd@...db.de>,
"Arnd Bergmann" <arnd@...nel.org>,
"Mark Brown" <broonie@...nel.org>,
"Wesley Cheng"
<quic_wcheng@...cinc.com>,
"Jaroslav Kysela" <perex@...ex.cz>,
"Takashi Iwai" <tiwai@...e.com>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
"Dan Carpenter" <dan.carpenter@...aro.org>,
<linux-sound@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion
On Wed, 17 Sep 2025 10:19:23 +0200,
Luca Weiss wrote:
>
> Hi Takashi,
>
> On Tue Sep 16, 2025 at 6:09 PM CEST, Takashi Iwai wrote:
> > On Tue, 16 Sep 2025 10:41:01 +0200,
> > Luca Weiss wrote:
> >>
> >> Hi Arnd,
> >>
> >> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
> >> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> >> >> On Fri, 05 Sep 2025 13:47:28 +0200,
> >> >
> >> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct
> >> >> snd_usb_substream *subs,
> >> >> if (!xfer_buf)
> >> >> return -ENOMEM;
> >> >>
> >> >> - /* Remapping is not possible if xfer_buf is outside of linear map */
> >> >> - xfer_buf_pa = virt_to_phys(xfer_buf);
> >> >> - if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> >> >> - ret = -ENXIO;
> >> >> - goto unmap_sync;
> >> >> - }
> >> >> dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >> >> xfer_buf_dma, len);
> >> >>
> >> >> /* map the physical buffer into sysdev as well */
> >> >> + /* note: 0 is passed to pa argument as we use sgt */
> >> >> xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> >> >> - xfer_buf_pa, len, &xfer_buf_sgt);
> >> >> + 0, len, &xfer_buf_sgt);
> >> >> if (!xfer_buf_dma_sysdev) {
> >> >> ret = -ENOMEM;
> >> >> goto unmap_sync;
> >> >
> >> >
> >> > Makes sense. I had to rework the code a little more to actually
> >> > understand how it fits together, for reference the below version
> >> > (I don't expect it to build cleanly) would split up
> >> > uaudio_iommu_map() into one function that takes a physical
> >> > address and another function that takes an sg table.
> >>
> >> Are you planning to post this as a proper patch? It's a bit late in the
> >> 6.17 cycle already but good to still get this fixed for final release.
> >>
> >> Or revert the original commit that broke it for now.
> >>
> >> I couldn't really test your patch since there's a couple of compile
> >> errors where I wasn't sure how to resolve them correctly.
> >
> > Could you check the patch below, then? At least it compiles without
> > errors.
>
> It does compile as well for me, but looks like it's not working.
>
> It's still triggering the WARN_ON from
>
> if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
>
> [ 214.157471] ------------[ cut here ]------------
> [ 214.157491] WARNING: CPU: 4 PID: 12 at sound/usb/qcom/qc_audio_offload.c:1067 handle_uaudio_stream_req+0xecc/0x13c4
> [ 214.157510] Modules linked in:
> [ 214.157522] CPU: 4 UID: 0 PID: 12 Comm: kworker/u32:0 Tainted: G W 6.16.0-00047-gfa3c1e37ba38 #1 NONE
> [ 214.157531] Tainted: [W]=WARN
> [ 214.157535] Hardware name: Fairphone 4 (DT)
> [ 214.157541] Workqueue: qmi_msg_handler qmi_data_ready_work
> [ 214.157553] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 214.157560] pc : handle_uaudio_stream_req+0xecc/0x13c4
> [ 214.157568] lr : handle_uaudio_stream_req+0xcdc/0x13c4
> [ 214.157575] sp : ffff8000800b39d0
> [ 214.157579] x29: ffff8000800b3b40 x28: ffff0000895eb6b0 x27: 0000000000008000
> [ 214.157589] x26: ffff0000d6bae960 x25: ffffa11dbe275e28 x24: 0000000000008000
> [ 214.157598] x23: ffffa11dbe0a4ec0 x22: ffff8000800b3c00 x21: 0000000000000000
> [ 214.157608] x20: ffff8000800b3cc8 x19: ffff00008b128ac0 x18: ffffa11dbdfaa258
> [ 214.157617] x17: ffff0000803e9388 x16: ffff0000800162c0 x15: ffffa11dbdfaa000
> [ 214.157626] x14: 0000000000000500 x13: ffffa11dbe03c000 x12: 0000000000000000
> [ 214.157636] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffa11dbe0f78a0
> [ 214.157645] x8 : ffffdee36909b000 x7 : ffff0000d6ac8418 x6 : 0000000000000000
> [ 214.157654] x5 : 0000000000000000 x4 : ffff8000800b3968 x3 : 0000000000000000
> [ 214.157663] x2 : 0000000000000000 x1 : ffff0000801a4100 x0 : 0000000000000000
> [ 214.157672] Call trace:
> [ 214.157677] handle_uaudio_stream_req+0xecc/0x13c4 (P)
> [ 214.157687] qmi_invoke_handler+0xb4/0x100
> [ 214.157694] qmi_handle_message+0x88/0x1a0
> [ 214.157702] qmi_data_ready_work+0x208/0x35c
> [ 214.157709] process_one_work+0x144/0x2c4
> [ 214.157719] worker_thread+0x280/0x45c
> [ 214.157726] kthread+0xfc/0x1dc
> [ 214.157733] ret_from_fork+0x10/0x20
> [ 214.157744] ---[ end trace 0000000000000000 ]---
>
> Should that code be removed with the new code now?
Yes, please try the revised patch below.
thanks,
Takashi
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
umap_size, iova, mapped_iova_size);
}
+static int uaudio_iommu_map_prot(bool dma_coherent)
+{
+ int prot = IOMMU_READ | IOMMU_WRITE;
+
+ if (dma_coherent)
+ prot |= IOMMU_CACHE;
+ return prot;
+}
+
/**
- * uaudio_iommu_map() - maps iommu memory for adsp
+ * uaudio_iommu_map_pa() - maps iommu memory for adsp
* @mtype: ring type
* @dma_coherent: dma coherent
* @pa: physical address for ring/buffer
* @size: size of memory region
- * @sgt: sg table for memory region
*
* Maps the XHCI related resources to a memory region that is assigned to be
* used by the adsp. This will be mapped to the domain, which is created by
* the ASoC USB backend driver.
*
*/
-static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
- phys_addr_t pa, size_t size,
- struct sg_table *sgt)
+static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
+ phys_addr_t pa, size_t size)
{
- struct scatterlist *sg;
unsigned long iova = 0;
- size_t total_len = 0;
- unsigned long iova_sg;
- phys_addr_t pa_sg;
bool map = true;
- size_t sg_len;
- int prot;
- int ret;
- int i;
-
- prot = IOMMU_READ | IOMMU_WRITE;
-
- if (dma_coherent)
- prot |= IOMMU_CACHE;
+ int prot = uaudio_iommu_map_prot(dma_coherent);
switch (mtype) {
case MEM_EVENT_RING:
@@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
&uaudio_qdev->xfer_ring_iova_size,
&uaudio_qdev->xfer_ring_list, size);
break;
- case MEM_XFER_BUF:
- iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
- &uaudio_qdev->xfer_buf_iova_size,
- &uaudio_qdev->xfer_buf_list, size);
- break;
default:
dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
}
if (!iova || !map)
- goto done;
+ return 0;
- if (!sgt)
- goto skip_sgt_map;
+ iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
+
+ return iova;
+}
+
+static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
+ struct sg_table *sgt)
+{
+ struct scatterlist *sg;
+ unsigned long iova = 0;
+ size_t total_len = 0;
+ unsigned long iova_sg;
+ phys_addr_t pa_sg;
+ size_t sg_len;
+ int prot = uaudio_iommu_map_prot(dma_coherent);
+ int ret;
+ int i;
+
+ prot = IOMMU_READ | IOMMU_WRITE;
+
+ if (dma_coherent)
+ prot |= IOMMU_CACHE;
+
+ iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
+ &uaudio_qdev->xfer_buf_iova_size,
+ &uaudio_qdev->xfer_buf_list, size);
+ if (!iova)
+ goto done;
iova_sg = iova;
for_each_sg(sgt->sgl, sg, sgt->nents, i) {
@@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
iova = 0;
}
- return iova;
-
-skip_sgt_map:
- iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
-
done:
return iova;
}
@@ -1020,7 +1031,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
struct sg_table xfer_buf_sgt;
dma_addr_t xfer_buf_dma;
void *xfer_buf;
- phys_addr_t xfer_buf_pa;
u32 len = xfer_buf_len;
bool dma_coherent;
dma_addr_t xfer_buf_dma_sysdev;
@@ -1051,18 +1061,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
if (!xfer_buf)
return -ENOMEM;
- /* Remapping is not possible if xfer_buf is outside of linear map */
- xfer_buf_pa = virt_to_phys(xfer_buf);
- if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
- ret = -ENXIO;
- goto unmap_sync;
- }
dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
xfer_buf_dma, len);
/* map the physical buffer into sysdev as well */
- xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
- xfer_buf_pa, len, &xfer_buf_sgt);
+ xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
+ len, &xfer_buf_sgt);
if (!xfer_buf_dma_sysdev) {
ret = -ENOMEM;
goto unmap_sync;
@@ -1143,8 +1147,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
sg_free_table(sgt);
/* data transfer ring */
- iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
- PAGE_SIZE, NULL);
+ iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
+ PAGE_SIZE);
if (!iova) {
ret = -ENOMEM;
goto clear_pa;
@@ -1207,8 +1211,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
mem_info->dma = sg_dma_address(sgt->sgl);
sg_free_table(sgt);
- iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
- PAGE_SIZE, NULL);
+ iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
+ PAGE_SIZE);
if (!iova) {
ret = -ENOMEM;
goto clear_pa;
Powered by blists - more mailing lists