* [PATCH] iommu/amd: Check if domain is NULL before dereference it
@ 2017-08-24 11:56 Baoquan He
2017-08-24 12:11 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Baoquan He @ 2017-08-24 11:56 UTC (permalink / raw)
To: iommu, linux-kernel; +Cc: dan.carpenter, joro, Baoquan He
In get_domain(), 'domain' could still be NULL before it's passed to
dma_ops_domain() to dereference. For safety, check if 'domain' is
NULL before passing to dma_ops_domain().
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Baoquan He <bhe@redhat.com>
---
drivers/iommu/amd_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 16f1e6af00b0..2e2d5e6a13b3 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct device *dev)
domain = to_pdomain(io_domain);
attach_device(dev, domain);
}
- if (!dma_ops_domain(domain))
+ if (domain && !dma_ops_domain(domain))
return ERR_PTR(-EBUSY);
return domain;
--
2.5.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 11:56 [PATCH] iommu/amd: Check if domain is NULL before dereference it Baoquan He
@ 2017-08-24 12:11 ` Dan Carpenter
2017-08-24 12:22 ` Baoquan He
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2017-08-24 12:11 UTC (permalink / raw)
To: Baoquan He; +Cc: iommu, linux-kernel, joro
On Thu, Aug 24, 2017 at 07:56:47PM +0800, Baoquan He wrote:
> In get_domain(), 'domain' could still be NULL before it's passed to
> dma_ops_domain() to dereference. For safety, check if 'domain' is
> NULL before passing to dma_ops_domain().
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> drivers/iommu/amd_iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 16f1e6af00b0..2e2d5e6a13b3 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct device *dev)
> domain = to_pdomain(io_domain);
> attach_device(dev, domain);
> }
> - if (!dma_ops_domain(domain))
> + if (domain && !dma_ops_domain(domain))
> return ERR_PTR(-EBUSY);
>
> return domain;
This still doesn't look right. None of the callers can handle a NULL
domain.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 12:11 ` Dan Carpenter
@ 2017-08-24 12:22 ` Baoquan He
2017-08-24 12:32 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Baoquan He @ 2017-08-24 12:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: iommu, linux-kernel, joro
On 08/24/17 at 03:11pm, Dan Carpenter wrote:
> On Thu, Aug 24, 2017 at 07:56:47PM +0800, Baoquan He wrote:
> > In get_domain(), 'domain' could still be NULL before it's passed to
> > dma_ops_domain() to dereference. For safety, check if 'domain' is
> > NULL before passing to dma_ops_domain().
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > drivers/iommu/amd_iommu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 16f1e6af00b0..2e2d5e6a13b3 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct device *dev)
> > domain = to_pdomain(io_domain);
> > attach_device(dev, domain);
> > }
> > - if (!dma_ops_domain(domain))
> > + if (domain && !dma_ops_domain(domain))
> > return ERR_PTR(-EBUSY);
> >
> > return domain;
>
> This still doesn't look right. None of the callers can handle a NULL
> domain.
Here the NULL domain is on purpose. In kdump kernel if iommu is
pre-enabled, just stop attach the device to domain until the device
driver init. So here in driver init when call get_domain(), if found
get_dev_data(dev)->defer_attach is true, we just do the attachment of
device to domain.
Not sure if I got what you mean about 'callers'.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 12:22 ` Baoquan He
@ 2017-08-24 12:32 ` Dan Carpenter
2017-08-24 12:47 ` Baoquan He
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2017-08-24 12:32 UTC (permalink / raw)
To: Baoquan He; +Cc: iommu, linux-kernel, joro
Take a look at this code for example. But all the places which call
get_domain() are the same:
drivers/iommu/amd_iommu.c
2648 page = virt_to_page(virt_addr);
2649 size = PAGE_ALIGN(size);
2650
2651 domain = get_domain(dev);
^^^^^^^^^^^^^^
imagined get_domain() returns NULL.
2652 if (IS_ERR(domain))
2653 goto free_mem;
2654
2655 dma_dom = to_dma_ops_domain(domain);
^^^^^^^^^^^^^^^^^^^^^^^^^
This will Oops.
2656
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 12:32 ` Dan Carpenter
@ 2017-08-24 12:47 ` Baoquan He
2017-08-24 12:53 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Baoquan He @ 2017-08-24 12:47 UTC (permalink / raw)
To: Dan Carpenter; +Cc: iommu, linux-kernel, joro
On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> Take a look at this code for example. But all the places which call
> get_domain() are the same:
>
> drivers/iommu/amd_iommu.c
> 2648 page = virt_to_page(virt_addr);
> 2649 size = PAGE_ALIGN(size);
> 2650
> 2651 domain = get_domain(dev);
> ^^^^^^^^^^^^^^
> imagined get_domain() returns NULL.
>
> 2652 if (IS_ERR(domain))
> 2653 goto free_mem;
> 2654
> 2655 dma_dom = to_dma_ops_domain(domain);
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> This will Oops.
I see, it's a problem. Thanks for telling!
How about below change? But I am not very sure which errno should be
picked, seems the latter one, EBUSY is better since it has passed the
check_device() checking.
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 16f1e6af00b0..2d7d04472555 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2262,6 +2262,9 @@ static struct protection_domain *get_domain(struct device *dev)
domain = to_pdomain(io_domain);
attach_device(dev, domain);
}
+ if (domain == NULL)
+ return ERR_PTR(-EBUSY);
+
if (!dma_ops_domain(domain))
return ERR_PTR(-EBUSY);
--
2.5.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 12:47 ` Baoquan He
@ 2017-08-24 12:53 ` Dan Carpenter
2017-08-24 13:01 ` Baoquan He
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2017-08-24 12:53 UTC (permalink / raw)
To: Baoquan He; +Cc: iommu, linux-kernel, joro
On Thu, Aug 24, 2017 at 08:47:33PM +0800, Baoquan He wrote:
> On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> > Take a look at this code for example. But all the places which call
> > get_domain() are the same:
> >
> > drivers/iommu/amd_iommu.c
> > 2648 page = virt_to_page(virt_addr);
> > 2649 size = PAGE_ALIGN(size);
> > 2650
> > 2651 domain = get_domain(dev);
> > ^^^^^^^^^^^^^^
> > imagined get_domain() returns NULL.
> >
> > 2652 if (IS_ERR(domain))
> > 2653 goto free_mem;
> > 2654
> > 2655 dma_dom = to_dma_ops_domain(domain);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^
> > This will Oops.
>
> I see, it's a problem. Thanks for telling!
>
> How about below change? But I am not very sure which errno should be
> picked, seems the latter one, EBUSY is better since it has passed the
> check_device() checking.
Looks good to me. You know better than I do which errno is best, so
I'll leave that to you.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
2017-08-24 12:53 ` Dan Carpenter
@ 2017-08-24 13:01 ` Baoquan He
0 siblings, 0 replies; 7+ messages in thread
From: Baoquan He @ 2017-08-24 13:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: iommu, linux-kernel, joro
On 08/24/17 at 03:53pm, Dan Carpenter wrote:
> On Thu, Aug 24, 2017 at 08:47:33PM +0800, Baoquan He wrote:
> > On 08/24/17 at 03:32pm, Dan Carpenter wrote:
> > > Take a look at this code for example. But all the places which call
> > > get_domain() are the same:
> > >
> > > drivers/iommu/amd_iommu.c
> > > 2648 page = virt_to_page(virt_addr);
> > > 2649 size = PAGE_ALIGN(size);
> > > 2650
> > > 2651 domain = get_domain(dev);
> > > ^^^^^^^^^^^^^^
> > > imagined get_domain() returns NULL.
> > >
> > > 2652 if (IS_ERR(domain))
> > > 2653 goto free_mem;
> > > 2654
> > > 2655 dma_dom = to_dma_ops_domain(domain);
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^
> > > This will Oops.
> >
> > I see, it's a problem. Thanks for telling!
> >
> > How about below change? But I am not very sure which errno should be
> > picked, seems the latter one, EBUSY is better since it has passed the
> > check_device() checking.
>
> Looks good to me. You know better than I do which errno is best, so
> I'll leave that to you.
OK, thanks! Then let me post v2 with it.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-08-24 13:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-24 11:56 [PATCH] iommu/amd: Check if domain is NULL before dereference it Baoquan He
2017-08-24 12:11 ` Dan Carpenter
2017-08-24 12:22 ` Baoquan He
2017-08-24 12:32 ` Dan Carpenter
2017-08-24 12:47 ` Baoquan He
2017-08-24 12:53 ` Dan Carpenter
2017-08-24 13:01 ` Baoquan He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox