[PATCH -next v2] iommu/amd: fix a null-ptr-deref in map_sg()

[PATCH -next v2] iommu/amd: fix a null-ptr-deref in map_sg()

[Qian Cai]

The commit 1a1079011da3 ("iommu/amd: Flush not present cache in
iommu_map_page") added domain_flush_np_cache() in map_sg() which
triggered a crash below during boot. sg_next() could return NULL if
sg_is_last() is true, so after for_each_sg(sglist, s, nelems, i), "s"
could be NULL which ends up deferencing a NULL pointer later here,

domain_flush_np_cache(domain, s->dma_address, s->dma_length);

so move domain_flush_np_cache() call inside for_each_sg() to loop over
each sg element.

BUG: kernel NULL pointer dereference, address: 0000000000000018
PGD 0 P4D 0
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN NOPTI
CPU: 8 PID: 659 Comm: kworker/8:1 Tainted: G    B
5.1.0-rc7-next-20190506+ #20
Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40
01/25/2019
Workqueue: events work_for_cpu_fn
RIP: 0010:map_sg+0x297/0x2e0
Call Trace:
 scsi_dma_map+0xc6/0x160
 pqi_raid_submit_scsi_cmd_with_io_request+0x3b4/0x470 [smartpqi]
 pqi_scsi_queue_command+0x791/0xdd0 [smartpqi]
 scsi_queue_rq+0x79c/0x1200
 blk_mq_dispatch_rq_list+0x4dc/0xb70
 blk_mq_sched_dispatch_requests+0x2e1/0x310
 __blk_mq_run_hw_queue+0x128/0x200
 __blk_mq_delay_run_hw_queue+0x2b7/0x2d0
 blk_mq_run_hw_queue+0x127/0x1d0
 blk_mq_sched_insert_request+0x25c/0x320
 __scsi_scan_target+0x14d/0x790
 scsi_scan_target+0x115/0x120
 sas_rphy_add+0x1d1/0x280 [scsi_transport_sas]
 pqi_add_sas_device+0x187/0x1e0 [smartpqi]
 pqi_update_device_list+0x1227/0x1460 [smartpqi]
 pqi_update_scsi_devices+0x755/0x1980 [smartpqi]
 pqi_scan_scsi_devices+0x57/0xf0 [smartpqi]
 pqi_ctrl_init+0x149e/0x14df [smartpqi]
 pqi_pci_probe.cold.49+0x808/0x818 [smartpqi]
 local_pci_probe+0x7a/0xc0
 work_for_cpu_fn+0x2e/0x50
 process_one_work+0x522/0xa10
 worker_thread+0x363/0x5b0
 kthread+0x1d2/0x1f0
 ret_from_fork+0x22/0x40

Signed-off-by: Qian Cai <cai@lca.pw>
---

v2: Call domain_flush_np_cache() inside for_each_sg().

 drivers/iommu/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 867f8b155000..b7132812ce59 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2576,9 +2576,9 @@ static int map_sg(struct device *dev, struct scatterlist *sglist,
 		 */
 		s->dma_address += address + (s->offset & ~PAGE_MASK);
 		s->dma_length   = s->length;
-	}
 
-	domain_flush_np_cache(domain, s->dma_address, s->dma_length);
+		domain_flush_np_cache(domain, s->dma_address, s->dma_length);
+	}
 
 	return nelems;
 
-- 
2.20.1 (Apple Git-117)

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH -next v2] iommu/amd: fix a null-ptr-deref in map_sg()

[Joerg Roedel]

Hi Qian,

On Mon, May 06, 2019 at 12:44:40PM -0400, Qian Cai wrote:
> The commit 1a1079011da3 ("iommu/amd: Flush not present cache in
> iommu_map_page") added domain_flush_np_cache() in map_sg() which
> triggered a crash below during boot. sg_next() could return NULL if
> sg_is_last() is true, so after for_each_sg(sglist, s, nelems, i), "s"
> could be NULL which ends up deferencing a NULL pointer later here,
> 
> domain_flush_np_cache(domain, s->dma_address, s->dma_length);
> 
> so move domain_flush_np_cache() call inside for_each_sg() to loop over
> each sg element.

Thanks for the fix, but it is too late to merge it into the tree. I am
going to revert commit 1a1079011da3 for now and we can try again in the
next cycle.


Thanks,

	Joerg

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH -next v2] iommu/amd: fix a null-ptr-deref in map_sg()

[Tom Murphy via iommu]

Hi Joerg,

Is there anything I need to do to get this patch into linux-next? My
patch to convert the amd iommu driver to use the dma-iommu ops depends
on this patch.

Thanks,
Tom

On Tue, May 7, 2019 at 8:39 AM Joerg Roedel <jroedel@suse.de> wrote:
>
> Hi Qian,
>
> On Mon, May 06, 2019 at 12:44:40PM -0400, Qian Cai wrote:
> > The commit 1a1079011da3 ("iommu/amd: Flush not present cache in
> > iommu_map_page") added domain_flush_np_cache() in map_sg() which
> > triggered a crash below during boot. sg_next() could return NULL if
> > sg_is_last() is true, so after for_each_sg(sglist, s, nelems, i), "s"
> > could be NULL which ends up deferencing a NULL pointer later here,
> >
> > domain_flush_np_cache(domain, s->dma_address, s->dma_length);
> >
> > so move domain_flush_np_cache() call inside for_each_sg() to loop over
> > each sg element.
>
> Thanks for the fix, but it is too late to merge it into the tree. I am
> going to revert commit 1a1079011da3 for now and we can try again in the
> next cycle.
>
>
> Thanks,
>
>         Joerg
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH -next v2] iommu/amd: fix a null-ptr-deref in map_sg()

[Joerg Roedel]

On Mon, May 06, 2019 at 12:44:40PM -0400, Qian Cai wrote:
> The commit 1a1079011da3 ("iommu/amd: Flush not present cache in
> iommu_map_page") 

That patch was reverted by me in

	97a18f548548a6ee1b9be14c6fc72090b8839875

because it caused issues by testers. So maybe re-submit the above patch
with this fix included?

Regards,

	Joerg

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu