iommu/amd: Check if domain is NULL before dereference it
PATCH 1/1 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().
--- drivers/iommu/amd_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/amd_iommu.c +1 -1
@@ -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;
Signature · 1 line
2.5.5
Quoted 15 lines · Baoquan He
In
get_domain(), 'domain' could still be NULL before it's passed todma_ops_domain()to dereference. For safety, check if 'domain' is NULL before passing todma_ops_domain().Reported-by Dan Carpenter <[email protected]>Signed-off-by Baoquan He <[email protected]>--- drivers/iommu/amd_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/amd_iommu.c +0
-0
drivers/iommu/amd_iommu.c static struct protection_domain *get_domain(struct device *dev) · line 2268
Quoted 8 lines
drivers/iommu/amd_iommu.c +1
-1@@ -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
Quoted 23 lines · Dan Carpenter
Quoted 22 lines · Baoquan He
In
get_domain(), 'domain' could still be NULL before it's passed todma_ops_domain()to dereference. For safety, check if 'domain' is NULL before passing todma_ops_domain().Reported-by Dan Carpenter <[email protected]>Signed-off-by Baoquan He <[email protected]>--- drivers/iommu/amd_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/amd_iommu.c +1
-1@@ -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 );
Quoted 4 lines
drivers/iommu/amd_iommu.c static struct protection_domain *get_domain(struct device *dev) · line 2268
Quoted 1 line
drivers/iommu/amd_iommu.c +0
-0return 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'.
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
Quoted 9 lines · Dan Carpenter
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);
26502651 domain = get_domain(dev); ^^^^^^^^^^^^^^
Quoted 8 lines
imagined
get_domain()returns NULL.2652 if (
IS_ERR(domain))
2653 gotofree_mem;
26542655 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.
drivers/iommu/amd_iommu.c +3 -0
@@ -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 );
Signature · 1 line
2.5.5
Quoted 19 lines · Baoquan He
Quoted 9 lines · Dan Carpenter
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);
26502651 domain = get_domain(dev); ^^^^^^^^^^^^^^Quoted 8 lines
imagined
get_domain()returns NULL.2652 if (
IS_ERR(domain))
2653 gotofree_mem;
26542655 dma_dom = to_dma_ops_domain(domain); ^^^^^^^^^^^^^^^^^^^^^^^^^This will Oops.
Quoted 5 lines
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
Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Baoquan He
Quoted 17 lines · Dan Carpenter
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);
26502651 domain = get_domain(dev); ^^^^^^^^^^^^^^imagined
get_domain()returns NULL.2652 if (
IS_ERR(domain))
2653 gotofree_mem;
26542655 dma_dom = to_dma_ops_domain(domain); ^^^^^^^^^^^^^^^^^^^^^^^^^This will Oops.
Quoted 8 lines
Quoted 5 lines
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.