[PATCH 0/4] Coverity fixes relating to xmalloc()/xfree()

[PATCH 0/4] Coverity fixes relating to xmalloc()/xfree()

[Andrew Cooper]

In the latest Coverity scan, I modelled _xmalloc() and xfree() which Coverity
was previously unaware of.  These four patches are as a result of this
improved modelling.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>

George/Ian: (I cant recall offhand when release-maintainership switches)

  I request these patches for a freeze exemption, on the basis that they are
  quite obviouly bugs with serious consequences, the fixes for which are
  contained and very unlikely to have accidential impact on Xen as a whole.

Andrew Cooper (4):
  common/sysctl: Don't leak status in SYSCTL_page_offline_op
  AMD/iommu_detect: Don't leak iommu structure on error paths
  AMD/microcode: Avoid use-after-free for the microcode buffer
  VTD/DMAR: free() correct pointer on error from acpi_parse_one_atsr()

 xen/arch/x86/microcode_amd.c               |   17 +++++++++--------
 xen/common/sysctl.c                        |   10 ++++------
 xen/drivers/passthrough/amd/iommu_detect.c |   11 ++++++++---
 xen/drivers/passthrough/vtd/dmar.c         |   18 +++++++++++++-----
 4 files changed, 34 insertions(+), 22 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Andrew Cooper]

Also fix the indentation of the arguments to copy_to_guest() to help clarify
that the 'ret = -EFAULT' is not part of the condition.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/sysctl.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 117e095..cd6184a 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -230,15 +230,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         }
 
         if ( copy_to_guest(
-            op->u.page_offline.status, status,
-            op->u.page_offline.end - op->u.page_offline.start + 1) )
-        {
+                 op->u.page_offline.status, status,
+                 op->u.page_offline.end - op->u.page_offline.start + 1) )
             ret = -EFAULT;
-            break;
-        }
+        else
+            copyback = 0;
 
         xfree(status);
-        copyback = 0;
     }
     break;
 
-- 
1.7.10.4

Re: [PATCH 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Jan Beulich]

>>> On 27.12.13 at 15:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Also fix the indentation of the arguments to copy_to_guest() to help clarify
> that the 'ret = -EFAULT' is not part of the condition.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/common/sysctl.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 117e095..cd6184a 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -230,15 +230,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          }
>  
>          if ( copy_to_guest(
> -            op->u.page_offline.status, status,
> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
> -        {
> +                 op->u.page_offline.status, status,
> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>              ret = -EFAULT;
> -            break;
> -        }
> +        else
> +            copyback = 0;
>  
>          xfree(status);
> -        copyback = 0;

This is wrong (and not covered by the title or description) - there's
nothing to copy back here (apart from "status"), so this should
remain unconditional.

Jan

Re: [PATCH 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Andrew Cooper]

On 07/01/14 11:33, Jan Beulich wrote:
>>>> On 27.12.13 at 15:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Also fix the indentation of the arguments to copy_to_guest() to help clarify
>> that the 'ret = -EFAULT' is not part of the condition.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  xen/common/sysctl.c |   10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index 117e095..cd6184a 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -230,15 +230,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>> u_sysctl)
>>          }
>>  
>>          if ( copy_to_guest(
>> -            op->u.page_offline.status, status,
>> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
>> -        {
>> +                 op->u.page_offline.status, status,
>> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>>              ret = -EFAULT;
>> -            break;
>> -        }
>> +        else
>> +            copyback = 0;
>>  
>>          xfree(status);
>> -        copyback = 0;
> This is wrong (and not covered by the title or description) - there's
> nothing to copy back here (apart from "status"), so this should
> remain unconditional.
>
> Jan
>

There is a 'break' removed from the if statement, so there is no change
to the conditions during which copyback gets set.

~Andrew

Re: [PATCH 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Jan Beulich]

>>> On 07.01.14 at 12:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/01/14 11:33, Jan Beulich wrote:
>>>>> On 27.12.13 at 15:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> Also fix the indentation of the arguments to copy_to_guest() to help clarify
>>> that the 'ret = -EFAULT' is not part of the condition.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> ---
>>>  xen/common/sysctl.c |   10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>>> index 117e095..cd6184a 100644
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -230,15 +230,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>>> u_sysctl)
>>>          }
>>>  
>>>          if ( copy_to_guest(
>>> -            op->u.page_offline.status, status,
>>> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
>>> -        {
>>> +                 op->u.page_offline.status, status,
>>> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>>>              ret = -EFAULT;
>>> -            break;
>>> -        }
>>> +        else
>>> +            copyback = 0;
>>>  
>>>          xfree(status);
>>> -        copyback = 0;
>> This is wrong (and not covered by the title or description) - there's
>> nothing to copy back here (apart from "status"), so this should
>> remain unconditional.
> 
> There is a 'break' removed from the if statement, so there is no change
> to the conditions during which copyback gets set.

Ah, true. But nevertheless, the code would be more correct if
the clearing of copyback was left where it was.

Jan

[Patch v2 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Andrew Cooper]

In addition, 'copyback' should be cleared even in the error case.

Also fix the indentation of the arguments to copy_to_guest() to help clarify
that the 'ret = -EFAULT' is not part of the condition.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---

Changes in v2:
 * Still clear copyback even in the error case.
---
 xen/common/sysctl.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 117e095..0cb6ee1 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -230,12 +230,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         }
 
         if ( copy_to_guest(
-            op->u.page_offline.status, status,
-            op->u.page_offline.end - op->u.page_offline.start + 1) )
-        {
+                 op->u.page_offline.status, status,
+                 op->u.page_offline.end - op->u.page_offline.start + 1) )
             ret = -EFAULT;
-            break;
-        }
 
         xfree(status);
         copyback = 0;
-- 
1.7.10.4

Re: [Patch v2 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Andrew Cooper]

On 07/01/14 11:59, Andrew Cooper wrote:
> In addition, 'copyback' should be cleared even in the error case.
>
> Also fix the indentation of the arguments to copy_to_guest() to help clarify
> that the 'ret = -EFAULT' is not part of the condition.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Ping?

>
> ---
>
> Changes in v2:
>  * Still clear copyback even in the error case.
> ---
>  xen/common/sysctl.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 117e095..0cb6ee1 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -230,12 +230,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>          }
>  
>          if ( copy_to_guest(
> -            op->u.page_offline.status, status,
> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
> -        {
> +                 op->u.page_offline.status, status,
> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>              ret = -EFAULT;
> -            break;
> -        }
>  
>          xfree(status);
>          copyback = 0;

Re: [Patch v2 1/4] common/sysctl: Don't leak status in SYSCTL_page_offline_op

[Keir Fraser]

On 07/01/2014 11:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> In addition, 'copyback' should be cleared even in the error case.
> 
> Also fix the indentation of the arguments to copy_to_guest() to help clarify
> that the 'ret = -EFAULT' is not part of the condition.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
> 
> Changes in v2:
>  * Still clear copyback even in the error case.
> ---
>  xen/common/sysctl.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 117e095..0cb6ee1 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -230,12 +230,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> u_sysctl)
>          }
>  
>          if ( copy_to_guest(
> -            op->u.page_offline.status, status,
> -            op->u.page_offline.end - op->u.page_offline.start + 1) )
> -        {
> +                 op->u.page_offline.status, status,
> +                 op->u.page_offline.end - op->u.page_offline.start + 1) )
>              ret = -EFAULT;
> -            break;
> -        }
>  
>          xfree(status);
>          copyback = 0;

[PATCH 2/4] AMD/iommu_detect: Don't leak iommu structure on error paths

[Andrew Cooper]

Tweak the logic slightly to return the real errors from
get_iommu_{,msi_}capabilities(), which at the moment is no functional change.

Coverity-ID: 1146950
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_detect.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c
index 6c10885..03003d7 100644
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -145,11 +145,11 @@ int __init amd_iommu_detect_one_acpi(
     rt = get_iommu_capabilities(iommu->seg, bus, dev, func,
                                 iommu->cap_offset, iommu);
     if ( rt )
-        return -ENODEV;
+        goto out;
 
     rt = get_iommu_msi_capabilities(iommu->seg, bus, dev, func, iommu);
     if ( rt )
-        return -ENODEV;
+        goto out;
 
     rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func));
     if ( rt )
@@ -158,6 +158,11 @@ int __init amd_iommu_detect_one_acpi(
                iommu->seg, bus, dev, func, rt);
 
     list_add_tail(&iommu->list, &amd_iommu_head);
+    rt = 0;
 
-    return 0;
+out:
+    if ( rt )
+        xfree(iommu);
+
+    return rt;
 }
-- 
1.7.10.4

Re: [PATCH 2/4] AMD/iommu_detect: Don't leak iommu structure on error paths

[Suravee Suthikulpanit]

On 12/27/2013 09:57 PM, Andrew Cooper wrote:
> Tweak the logic slightly to return the real errors from
> get_iommu_{,msi_}capabilities(), which at the moment is no functional change.
>
> Coverity-ID: 1146950
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/drivers/passthrough/amd/iommu_detect.c |   11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c
> index 6c10885..03003d7 100644
> --- a/xen/drivers/passthrough/amd/iommu_detect.c
> +++ b/xen/drivers/passthrough/amd/iommu_detect.c
> @@ -145,11 +145,11 @@ int __init amd_iommu_detect_one_acpi(
>       rt = get_iommu_capabilities(iommu->seg, bus, dev, func,
>                                   iommu->cap_offset, iommu);
>       if ( rt )
> -        return -ENODEV;
> +        goto out;
>
>       rt = get_iommu_msi_capabilities(iommu->seg, bus, dev, func, iommu);
>       if ( rt )
> -        return -ENODEV;
> +        goto out;
>
>       rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func));
>       if ( rt )
> @@ -158,6 +158,11 @@ int __init amd_iommu_detect_one_acpi(
>                  iommu->seg, bus, dev, func, rt);
>
>       list_add_tail(&iommu->list, &amd_iommu_head);
> +    rt = 0;
>
> -    return 0;
> +out:
> +    if ( rt )
> +        xfree(iommu);
> +
> +    return rt;
>   }
>
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

[PATCH 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Andrew Cooper]

It is possible to free the mc_old buffer and the store it for use on in the
case of resume.

This keeps the old semantics of being able to return an error even after a
successful microcode application.

Coverity-ID 1146953
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/microcode_amd.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index a3ceef8..8ea4e63 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
     struct microcode_amd *mc_amd, *mc_old;
     size_t offset = bufsize;
     size_t last_offset, applied_offset = 0;
-    int error = 0;
+    int error = 0, save_error = 1;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
 
     /* We should bind the task to the CPU */
@@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
      */
     if ( applied_offset )
     {
-        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
-                                            &applied_offset);
-        if ( ret == 0 )
-            xfree(mc_old);
-        else
-            error = ret;
+        save_error = get_ucode_from_buffer_amd(
+            mc_amd, buf, bufsize, &applied_offset);
+
+        if ( save_error )
+            error = save_error
     }
 
-    if ( !applied_offset || error )
+    if ( save_error )
     {
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }
+    else
+        xfree(mc_old);
 
   out:
     svm_host_osvw_init();
-- 
1.7.10.4

Re: [PATCH 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Boris Ostrovsky]

On 12/27/2013 09:57 AM, Andrew Cooper wrote:
> It is possible to free the mc_old buffer and the store it for use on in the

I think you don't need *the* store here. And no *on*.

> case of resume.
>
> This keeps the old semantics of being able to return an error even after a
> successful microcode application.
>
> Coverity-ID 1146953
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/arch/x86/microcode_amd.c |   17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index a3ceef8..8ea4e63 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>       struct microcode_amd *mc_amd, *mc_old;
>       size_t offset = bufsize;
>       size_t last_offset, applied_offset = 0;
> -    int error = 0;
> +    int error = 0, save_error = 1;
>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>   
>       /* We should bind the task to the CPU */
> @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>        */
>       if ( applied_offset )
>       {
> -        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
> -                                            &applied_offset);
> -        if ( ret == 0 )
> -            xfree(mc_old);
> -        else
> -            error = ret;
> +        save_error = get_ucode_from_buffer_amd(
> +            mc_amd, buf, bufsize, &applied_offset);
> +
> +        if ( save_error )
> +            error = save_error
>       }
>   
> -    if ( !applied_offset || error )
> +    if ( save_error )
>       {
>           xfree(mc_amd);
>           uci->mc.mc_amd = mc_old;
>       }
> +    else
> +        xfree(mc_old);

Won't this free mc_old (which is where previous microcode lived) even if 
the new buffer didn't have any valid microcode (i.e. applied_offset is 
zero)?

-boris


>   
>     out:
>       svm_host_osvw_init();

Re: [PATCH 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Andrew Cooper]

On 27/12/2013 15:30, Boris Ostrovsky wrote:
> On 12/27/2013 09:57 AM, Andrew Cooper wrote:
>> It is possible to free the mc_old buffer and the store it for use on
>> in the
>
> I think you don't need *the* store here. And no *on*.

Oops - I refactored my sentence half way through, and missed that on a
reread.

>
>> case of resume.
>>
>> This keeps the old semantics of being able to return an error even
>> after a
>> successful microcode application.
>>
>> Coverity-ID 1146953
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   xen/arch/x86/microcode_amd.c |   17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index a3ceef8..8ea4e63 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const
>> void *buf, size_t bufsize)
>>       struct microcode_amd *mc_amd, *mc_old;
>>       size_t offset = bufsize;
>>       size_t last_offset, applied_offset = 0;
>> -    int error = 0;
>> +    int error = 0, save_error = 1;
>>       struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>         /* We should bind the task to the CPU */
>> @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const
>> void *buf, size_t bufsize)
>>        */
>>       if ( applied_offset )
>>       {
>> -        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>> -                                            &applied_offset);
>> -        if ( ret == 0 )
>> -            xfree(mc_old);
>> -        else
>> -            error = ret;
>> +        save_error = get_ucode_from_buffer_amd(
>> +            mc_amd, buf, bufsize, &applied_offset);
>> +
>> +        if ( save_error )
>> +            error = save_error
>>       }
>>   -    if ( !applied_offset || error )
>> +    if ( save_error )
>>       {
>>           xfree(mc_amd);
>>           uci->mc.mc_amd = mc_old;
>>       }
>> +    else
>> +        xfree(mc_old);
>
> Won't this free mc_old (which is where previous microcode lived) even
> if the new buffer didn't have any valid microcode (i.e. applied_offset
> is zero)?
>
> -boris

save_error starts off as 1, and only gets set to 0 on a successful
get_ucode_from_buffer_amd(), so we will only free mc_old in the case
that we have valid microcde in mc_amd.  All other cases free mc_amd and
revert to using mc_old.

~Andrew

>
>
>>       out:
>>       svm_host_osvw_init();
>

Re: [PATCH 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Boris Ostrovsky]

On 12/27/2013 10:36 AM, Andrew Cooper wrote:
> On 27/12/2013 15:30, Boris Ostrovsky wrote:
>> On 12/27/2013 09:57 AM, Andrew Cooper wrote:
>>> It is possible to free the mc_old buffer and the store it for use on
>>> in the
>> I think you don't need *the* store here. And no *on*.
> Oops - I refactored my sentence half way through, and missed that on a
> reread.
>
>>> case of resume.
>>>
>>> This keeps the old semantics of being able to return an error even
>>> after a
>>> successful microcode application.
>>>
>>> Coverity-ID 1146953
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> ---
>>>    xen/arch/x86/microcode_amd.c |   17 +++++++++--------
>>>    1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>>> index a3ceef8..8ea4e63 100644
>>> --- a/xen/arch/x86/microcode_amd.c
>>> +++ b/xen/arch/x86/microcode_amd.c
>>> @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const
>>> void *buf, size_t bufsize)
>>>        struct microcode_amd *mc_amd, *mc_old;
>>>        size_t offset = bufsize;
>>>        size_t last_offset, applied_offset = 0;
>>> -    int error = 0;
>>> +    int error = 0, save_error = 1;
>>>        struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>>          /* We should bind the task to the CPU */
>>> @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const
>>> void *buf, size_t bufsize)
>>>         */
>>>        if ( applied_offset )
>>>        {
>>> -        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>> -                                            &applied_offset);
>>> -        if ( ret == 0 )
>>> -            xfree(mc_old);
>>> -        else
>>> -            error = ret;
>>> +        save_error = get_ucode_from_buffer_amd(
>>> +            mc_amd, buf, bufsize, &applied_offset);
>>> +
>>> +        if ( save_error )
>>> +            error = save_error
>>>        }
>>>    -    if ( !applied_offset || error )
>>> +    if ( save_error )
>>>        {
>>>            xfree(mc_amd);
>>>            uci->mc.mc_amd = mc_old;
>>>        }
>>> +    else
>>> +        xfree(mc_old);
>> Won't this free mc_old (which is where previous microcode lived) even
>> if the new buffer didn't have any valid microcode (i.e. applied_offset
>> is zero)?
>>
>> -boris
> save_error starts off as 1, and only gets set to 0 on a successful
> get_ucode_from_buffer_amd(), so we will only free mc_old in the case
> that we have valid microcde in mc_amd.  All other cases free mc_amd and
> revert to using mc_old.
>
> ~Andrew


Ah, I missed that save_error is initialized to 1.

Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>
>>
>>>        out:
>>>        svm_host_osvw_init();

[Patch v2 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Andrew Cooper]

It is possible to free the mc_old buffer and then store it for use in the case
of resume.

This keeps the old semantics of being able to return an error even after a
successful microcode application.

Coverity-ID 1146953
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---
Changes in v2:
 * Fix commit message.  No code change.
---
 xen/arch/x86/microcode_amd.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index a3ceef8..8ea4e63 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
     struct microcode_amd *mc_amd, *mc_old;
     size_t offset = bufsize;
     size_t last_offset, applied_offset = 0;
-    int error = 0;
+    int error = 0, save_error = 1;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
 
     /* We should bind the task to the CPU */
@@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
      */
     if ( applied_offset )
     {
-        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
-                                            &applied_offset);
-        if ( ret == 0 )
-            xfree(mc_old);
-        else
-            error = ret;
+        save_error = get_ucode_from_buffer_amd(
+            mc_amd, buf, bufsize, &applied_offset);
+
+        if ( save_error )
+            error = save_error
     }
 
-    if ( !applied_offset || error )
+    if ( save_error )
     {
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }
+    else
+        xfree(mc_old);
 
   out:
     svm_host_osvw_init();
-- 
1.7.10.4

Re: [Patch v2 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Matthew Daley]

[-- Attachment #1.1: Type: text/plain, Size: 2316 bytes --]

On Sat, Dec 28, 2013 at 4:57 AM, Andrew Cooper <andrew.cooper3@citrix.com>wrote:

> It is possible to free the mc_old buffer and then store it for use in the
> case
> of resume.
>
> This keeps the old semantics of being able to return an error even after a
> successful microcode application.
>
> Coverity-ID 1146953
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> ---
> Changes in v2:
>  * Fix commit message.  No code change.
> ---
>  xen/arch/x86/microcode_amd.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index a3ceef8..8ea4e63 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const void
> *buf, size_t bufsize)
>      struct microcode_amd *mc_amd, *mc_old;
>      size_t offset = bufsize;
>      size_t last_offset, applied_offset = 0;
> -    int error = 0;
> +    int error = 0, save_error = 1;
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>
>      /* We should bind the task to the CPU */
> @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const void
> *buf, size_t bufsize)
>       */
>      if ( applied_offset )
>      {
> -        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
> -                                            &applied_offset);
> -        if ( ret == 0 )
> -            xfree(mc_old);
> -        else
> -            error = ret;
> +        save_error = get_ucode_from_buffer_amd(
> +            mc_amd, buf, bufsize, &applied_offset);
> +
> +        if ( save_error )
> +            error = save_error
>

Missing you-know-what.

- Matthew


>      }
>
> -    if ( !applied_offset || error )
> +    if ( save_error )
>      {
>          xfree(mc_amd);
>          uci->mc.mc_amd = mc_old;
>      }
> +    else
> +        xfree(mc_old);
>
>    out:
>      svm_host_osvw_init();
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 3583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Patch v2 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2487 bytes --]

On 27/12/2013 22:43, Matthew Daley wrote:
> On Sat, Dec 28, 2013 at 4:57 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     It is possible to free the mc_old buffer and then store it for use
>     in the case
>     of resume.
>
>     This keeps the old semantics of being able to return an error even
>     after a
>     successful microcode application.
>
>     Coverity-ID 1146953
>     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com
>     <mailto:andrew.cooper3@citrix.com>>
>     CC: Keir Fraser <keir@xen.org <mailto:keir@xen.org>>
>     CC: Jan Beulich <JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com
>     <mailto:boris.ostrovsky@oracle.com>>
>     CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com
>     <mailto:suravee.suthikulpanit@amd.com>>
>
>     ---
>     Changes in v2:
>      * Fix commit message.  No code change.
>     ---
>      xen/arch/x86/microcode_amd.c |   17 +++++++++--------
>      1 file changed, 9 insertions(+), 8 deletions(-)
>
>     diff --git a/xen/arch/x86/microcode_amd.c
>     b/xen/arch/x86/microcode_amd.c
>     index a3ceef8..8ea4e63 100644
>     --- a/xen/arch/x86/microcode_amd.c
>     +++ b/xen/arch/x86/microcode_amd.c
>     @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu,
>     const void *buf, size_t bufsize)
>          struct microcode_amd *mc_amd, *mc_old;
>          size_t offset = bufsize;
>          size_t last_offset, applied_offset = 0;
>     -    int error = 0;
>     +    int error = 0, save_error = 1;
>          struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>
>          /* We should bind the task to the CPU */
>     @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu,
>     const void *buf, size_t bufsize)
>           */
>          if ( applied_offset )
>          {
>     -        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>     -                                            &applied_offset);
>     -        if ( ret == 0 )
>     -            xfree(mc_old);
>     -        else
>     -            error = ret;
>     +        save_error = get_ucode_from_buffer_amd(
>     +            mc_amd, buf, bufsize, &applied_offset);
>     +
>     +        if ( save_error )
>     +            error = save_error
>
>
> Missing you-know-what.
>
> - Matthew

I really should learn that "trivial tweaks" are often not quite so
trivial. v3 on its way.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5367 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Patch v3 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer

[Andrew Cooper]

It is possible to free the mc_old buffer and then store it for use in the case
of resume.

This keeps the old semantics of being able to return an error even after a
successful microcode application.

Coverity-ID 1146953
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
Acked-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

---
Changes in v3:
 * Insert missing semicolon.
Changes in v2:
 * Fix commit message.  No code change.
---
 xen/arch/x86/microcode_amd.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index a3ceef8..3014245 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
     struct microcode_amd *mc_amd, *mc_old;
     size_t offset = bufsize;
     size_t last_offset, applied_offset = 0;
-    int error = 0;
+    int error = 0, save_error = 1;
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
 
     /* We should bind the task to the CPU */
@@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
      */
     if ( applied_offset )
     {
-        int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
-                                            &applied_offset);
-        if ( ret == 0 )
-            xfree(mc_old);
-        else
-            error = ret;
+        save_error = get_ucode_from_buffer_amd(
+            mc_amd, buf, bufsize, &applied_offset);
+
+        if ( save_error )
+            error = save_error;
     }
 
-    if ( !applied_offset || error )
+    if ( save_error )
     {
         xfree(mc_amd);
         uci->mc.mc_amd = mc_old;
     }
+    else
+        xfree(mc_old);
 
   out:
     svm_host_osvw_init();
-- 
1.7.10.4

[PATCH 4/4] VTD/DMAR: free() correct pointer on error from acpi_parse_one_atsr()

[Andrew Cooper]

Free the allocated structure rather than the ACPI table ATS entry.

On further analysis, there is another memory leak.  acpi_parse_dev_scope()
could allocate scope->devices, and return with -ENOMEM.  All callers of
acpi_parse_dev_scope() would then free the underlying structure, loosing the
pointer.

These errors can only actually be reached through acpi_parse_dev_scope()
(which passes type = DMAR_TYPE), but I am quite surprised Coverity didn't spot
it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity-ID: 1146949
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Xiantao Zhang <xiantao.zhang@intel.com>
---
 xen/drivers/passthrough/vtd/dmar.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 8e162ff..1907496 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -312,7 +312,7 @@ static int __init acpi_parse_dev_scope(
     const struct acpi_dmar_pci_path *path;
     struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
         container_of(scope, struct acpi_drhd_unit, scope) : NULL;
-    int depth, cnt, didx = 0;
+    int depth, cnt, didx = 0, ret;
 
     if ( (cnt = scope_device_count(start, end)) < 0 )
         return cnt;
@@ -364,9 +364,10 @@ static int __init acpi_parse_dev_scope(
             {
                 struct acpi_hpet_unit *acpi_hpet_unit;
 
+                ret = -ENOMEM;
                 acpi_hpet_unit = xmalloc(struct acpi_hpet_unit);
                 if ( !acpi_hpet_unit )
-                    return -ENOMEM;
+                    goto out;
                 acpi_hpet_unit->id = acpi_scope->enumeration_id;
                 acpi_hpet_unit->bus = bus;
                 acpi_hpet_unit->dev = path->dev;
@@ -397,9 +398,10 @@ static int __init acpi_parse_dev_scope(
 
             if ( drhd )
             {
+                ret = -ENOMEM;
                 acpi_ioapic_unit = xmalloc(struct acpi_ioapic_unit);
                 if ( !acpi_ioapic_unit )
-                    return -ENOMEM;
+                    goto out;
                 acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
                 acpi_ioapic_unit->ioapic.bdf.bus = bus;
                 acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
@@ -420,7 +422,13 @@ static int __init acpi_parse_dev_scope(
         start += acpi_scope->length;
    }
 
-    return 0;
+    ret = 0;
+
+out:
+    if ( ret )
+        xfree(scope->devices);
+
+    return ret;
 }
 
 static int __init acpi_dmar_check_length(
@@ -708,7 +716,7 @@ acpi_parse_one_atsr(struct acpi_dmar_header *header)
     }
 
     if ( ret )
-        xfree(atsr);
+        xfree(atsru);
     else
         acpi_register_atsr_unit(atsru);
     return ret;
-- 
1.7.10.4