[PATCH] passthrough/amd: avoid reading an uninitialized variable.

[PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Tim Deegan]

update_intremap_entry_from_msi() doesn't write to its data pointer on
some error paths, so we copying that variable into the msg would count
as undefined behaviour.

Signed-off-by: Tim Deegan <tim@xen.org>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
I'm not sure whether we ought to be writing some default value
instead.  Happy to respin if someone knowledgeable can advise.
---
 xen/drivers/passthrough/amd/iommu_intr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index c1b76fb..c097d52 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -529,10 +529,11 @@ int amd_iommu_msi_msg_update_ire(
     } while ( PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
 
     if ( !rc )
+    {
         for ( i = 1; i < nr; ++i )
             msi_desc[i].remap_index = msi_desc->remap_index + i;
-
-    msg->data = data;
+        msg->data = data;
+    }
     return rc;
 }
 
-- 
2.1.4

Re: [PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Andrew Cooper]

On 16/04/15 17:44, Tim Deegan wrote:
> update_intremap_entry_from_msi() doesn't write to its data pointer on
> some error paths, so we copying that variable into the msg would count
> as undefined behaviour.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

This looks curiously familiar.

http://lists.xen.org/archives/html/xen-devel/2015-03/msg03679.html

I was meaning to get around to submitting it as a formal patch, but you
have definitely beaten me to it :)

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Jan Beulich]

>>> On 16.04.15 at 19:19, <andrew.cooper3@citrix.com> wrote:
> On 16/04/15 17:44, Tim Deegan wrote:
>> update_intremap_entry_from_msi() doesn't write to its data pointer on
>> some error paths, so we copying that variable into the msg would count
>> as undefined behaviour.
>>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> 
> This looks curiously familiar.
> 
> http://lists.xen.org/archives/html/xen-devel/2015-03/msg03679.html 
> 
> I was meaning to get around to submitting it as a formal patch, but you
> have definitely beaten me to it :)

But sadly it didn't help with the issue Sander reported (and I also
didn't expect it to, as errors here should cause the caller to not
use msg at all, and the regressing patch didn't alter anything that
could have caused errors to surface where none where seen
before). Still the patch is a good one as correcting a language
spec violation.

Jan

Re: [PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Konrad Rzeszutek Wilk]

On Thu, Apr 16, 2015 at 05:44:10PM +0100, Tim Deegan wrote:
> update_intremap_entry_from_msi() doesn't write to its data pointer on
> some error paths, so we copying that variable into the msg would count
> as undefined behaviour.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

CC-ing Sander who I believe saw this problem.
http://lists.xen.org/archives/html/xen-devel/2015-03/msg03679.html

> ---
> I'm not sure whether we ought to be writing some default value
> instead.  Happy to respin if someone knowledgeable can advise.
> ---
>  xen/drivers/passthrough/amd/iommu_intr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
> index c1b76fb..c097d52 100644
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -529,10 +529,11 @@ int amd_iommu_msi_msg_update_ire(
>      } while ( PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
>  
>      if ( !rc )
> +    {
>          for ( i = 1; i < nr; ++i )
>              msi_desc[i].remap_index = msi_desc->remap_index + i;
> -
> -    msg->data = data;
> +        msg->data = data;
> +    }
>      return rc;
>  }
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Tim Deegan]

At 17:44 +0100 on 16 Apr (1429206250), Tim Deegan wrote:
> update_intremap_entry_from_msi() doesn't write to its data pointer on
> some error paths, so we copying that variable into the msg would count
> as undefined behaviour.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

AMD maintainers: can you ack/nack this please?

Cheers,

Tim.

Re: [PATCH] passthrough/amd: avoid reading an uninitialized variable.

[Suthikulpanit, Suravee]

On 4/23/15, 04:25, "Tim Deegan" <tim@xen.org> wrote:

>At 17:44 +0100 on 16 Apr (1429206250), Tim Deegan wrote:
>> update_intremap_entry_from_msi() doesn't write to its data pointer on
>> some error paths, so we copying that variable into the msg would count
>> as undefined behaviour.
>> 
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>
>AMD maintainers: can you ack/nack this please?
>
>Cheers,
>
>Tim.

Acked-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Thanks,

Suravee