Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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> > Signed-off-by: Baoquan He > > --- > > 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'. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
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> Signed-off-by: Baoquan He > --- > 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: Check if domain is NULL before dereference it
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 CarpenterSigned-off-by: Baoquan He --- 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu