iommu/vtd: Fix NULL pointer dereference in prq_event_thread()
PATCH 1/1 iommu/vtd: Fix NULL pointer dereference in prq_event_thread()
When handling page request without pasid event, go to "no_pasid" branch instead of "bad_req". Otherwise, a NULL pointer deference will happen there.
--- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel-svm.c +1 -1
@@ -595,7 +595,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)pr_err ("%s: Page request without PASID: %08llx %08llx\n" , iommu->name, ((unsigned long long *)req)[0 ], ((unsigned long long *)req)[1 ]); - goto bad_req; + goto no_pasid; }if (!svm || svm->pasid != req->pasid) {
Signature · 1 line
2.17.1
Hi Baolu,
Quoted 24 lines
From: [email protected] [mailto:iommu- [email protected]] On Behalf Of Lu Baolu
Sent: Monday, November 5, 2018 10:19 AM
To: Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>
Cc: Raj, Ashok <[email protected]>; [email protected]; [email protected]
Subject: [PATCH 1/1] iommu/vtd: Fix NULL pointer dereference inprq_event_thread()When handling page request without pasid event, go to "
no_pasid" branch instead of "bad_req". Otherwise, a NULL pointer deference will happen there.Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Sohil Mehta <[email protected]>Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/intel-svm.c index renamed from drivers/iommu/intel-svm.c +0
-0db301efe126d..887150907526 100644
drivers/iommu/intel-svm.c static irqreturn_t prq_event_thread(int irq, void *d)
Quoted 9 lines
drivers/iommu/intel-svm.c +1
-2@@ -595,7 +595,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)pr_err ("%s: Page request without PASID: %08llx %08llx\n" , iommu->name, ((unsigned long long *)req)[0 ], ((unsigned long long *)req)[1 ]); - goto bad_req; + goto no_pasid; }if (!svm || svm->pasid != req->pasid) { --
I'm afraid it is still necessary to goto "bad_req". The following code behind "bad_req" will trigger fault_cb registered by in-kernel drivers. It is reasonable that PRQ without PASID can be handled by such callbacks. So I would suggest to keep the existing logic.
if (sdev && sdev->ops && sdev->ops->fault_cb) {
int rwxp = (req->rd_req << 3) | (req->wr_req << 2) |
(req->exe_req << 1) | (req->priv_req);
sdev->ops->fault_cb(sdev->dev, req->pasid, req->addr, req->private, rwxp, result);
}Thanks,
Yi Liu
Hi Yi,
Quoted 33 lines · Yi L
Hi Baolu,
Quoted 31 lines
From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu- bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Lu Baolu
Sent: Monday, November 5, 2018 10:19 AM
To: Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>
Cc: Raj, Ashok <[email protected]>; [email protected]; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Subject: [PATCH 1/1] iommu/vtd: Fix NULL pointer dereference inprq_event_thread()When handling page request without pasid event, go to "
no_pasid" branch instead of "bad_req". Otherwise, a NULL pointer deference will happen there.Cc Ashok Raj <[email protected]>Cc Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>Cc Sohil Mehta <[email protected]>--- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/intel-svm.c index renamed from drivers/iommu/intel-svm.c +0
-0db301efe126d..887150907526 100644
drivers/iommu/intel-svm.c +1
-1@@ -595,7 +595,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)pr_err ("%s: Page request without PASID: %08llx %08llx\n" , iommu->name, ((unsigned long long *)req)[0 ], ((unsigned long long *)req)[1 ]); - goto bad_req; + goto no_pasid; }
Quoted 7 lines
drivers/iommu/intel-svm.c static irqreturn_t prq_event_thread(int irq, void *d)
Quoted 2 lines
drivers/iommu/intel-svm.c +0
-1if (!svm || svm->pasid != req->pasid) { --I'm afraid it is still necessary to goto "
bad_req". The following code behind "bad_req" will triggerfault_cbregistered by in-kernel drivers. It is reasonable that PRQ without PASID can be handled by such callbacks. So I would suggest to keep the existing logic.
A page fault without a pasid is triggered by a DMA transfer without PASID. It doesn't relate to the SVM functionality hence there's no @svm or @sdev related to it. It's unnecessary to report it to the drivers as far as I can see.
Best regards,
Lu Baolu
Quoted 10 lines
if (sdev && sdev->ops && sdev->ops->fault_cb) { int rwxp = (req->rd_req << 3) | (req->wr_req << 2) | (req->exe_req << 1) | (req->priv_req); sdev->ops->fault_cb(sdev->dev, req->pasid, req->addr, req->private, rwxp, result); }Thanks,
Yi Liu
Quoted 57 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:45 PM
To: Liu, Yi L <[email protected]>; Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>
Cc: baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]; Raj, Ashok <[email protected]>; linux- [email protected]; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Subject: Re: [PATCH 1/1] iommu/vtd: Fix NULL pointer dereference inprq_event_thread()Hi Yi,
Quoted 14 lines · Yi L
Hi Baolu,
Quoted 12 lines
From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu- bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Lu Baolu
Sent: Monday, November 5, 2018 10:19 AM
To: Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>
Cc: Raj, Ashok <[email protected]>; [email protected]; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Subject: [PATCH 1/1] iommu/vtd: Fix NULL pointer dereference inprq_event_thread()When handling page request without pasid event, go to "
no_pasid" branch instead of "bad_req". Otherwise, a NULL pointer deference will happenthere.
Quoted 22 lines
Quoted 22 lines
Cc Ashok Raj <[email protected]>Cc Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>Cc Sohil Mehta <[email protected]>--- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/intel-svm.c +0
-0index
db301efe126d..887150907526 100644drivers/iommu/intel-svm.c +1
-1@@ -595,7 +595,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)pr_err ("%s: Page request without PASID: %08llx %08llx\n" , iommu->name, ((unsigned long long *)req)[0 ], ((unsigned long long *)req)[1 ]); - goto bad_req; + goto no_pasid; }Quoted 7 lines
drivers/iommu/intel-svm.c static irqreturn_t prq_event_thread(int irq, void *d)
Quoted 2 lines
drivers/iommu/intel-svm.c +0
-1if (!svm || svm->pasid != req->pasid) { --I'm afraid it is still necessary to goto "
bad_req". The following code behind "bad_req" will triggerfault_cbregistered by in-kernel drivers. It is reasonable that PRQ without PASID can be handled by such callbacks. So I would suggest to keep the existing logic.
Quoted 3 lines
A page fault without a pasid is triggered by a DMA transfer without PASID. It doesn't relate to the SVM functionality hence there's no @svm or @sdev related to it. It's unnecessary to report it to the drivers as far as I can see.
Yeah, a PRQ without PASID has no corresponding svm or sdev structure. Regards to this fact, it's acceptable for this fix. In long term, it might be helpful to refine the PRQ event handler to cover PRQ without PASID. I guess Jacob's fault report framework may help.
+Jacob
Regards,
Yi Liu
Joerg Roedel
maintainerQuoted 11 lines · Lu Baolu
When handling page request without pasid event, go to "
no_pasid" branch instead of "bad_req". Otherwise, a NULL pointer deference will happen there.Cc Ashok Raj <[email protected]>Cc Jacob Pan <[email protected]>Cc Sohil Mehta <[email protected]>Signed-off-by Lu Baolu <[email protected]>--- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied for v4.20, thanks.