Xen devel - Sep 2013 - [PATCH 0/4] Misc Xen fixes identified by Coverity

Hello,

Here are 4 largely unrelated patches fixing issues identified by Coverity in
Xen.

I hope they are all uncontentious.

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

So NULL checks are logically dead code.

Coverity IDs: 1055247, 1087147

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hvm/vmsi.c |    2 --
 xen/arch/x86/irq.c      |    2 --
 2 files changed, 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 4826b4a..b0766a4 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -304,8 +304,6 @@ static int msixtbl_write(struct vcpu *v, unsigned long
address,
         goto out;
     
     desc = irq_to_desc(msi_desc->irq);
-    if ( !desc )
-        goto out;
 
     spin_lock_irqsave(&desc->lock, flags);
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index c61cc46..e6036ec 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -646,8 +646,6 @@ void irq_move_cleanup_interrupt(struct cpu_user_regs *regs)
             continue;
 
         desc = irq_to_desc(irq);
-        if (!desc)
-            continue;
 
         spin_lock(&desc->lock);
         if (!desc->arch.move_cleanup_count)
-- 
1.7.10.4

Coverity ID: 1055622

Coverity correctly points out that this ASSERT() is unconditionally true as an
unsigned integer is always >= 0.

Judging from the shadow counterpart and p2m callsites, there is nothing
invalid about freeing the final p2m page.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/hap/hap.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index bff05d9..d3f64bd 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -289,7 +289,6 @@ static void hap_free_p2m_page(struct domain *d, struct
page_info *pg)
     d->arch.paging.hap.p2m_pages--;
     d->arch.paging.hap.total_pages++;
     hap_free(d, page_to_mfn(pg));
-    ASSERT(d->arch.paging.hap.p2m_pages >= 0);
 
     paging_unlock(d);
 }
-- 
1.7.10.4

Coverity ID 1055622, 1055630

node is an unsigned integer, so neccesserily >= 0.  There does not appear to
be any purpose to these assertions in the first place.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/common/page_alloc.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 41251b2..8cab81b 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -585,7 +585,6 @@ static struct page_info *alloc_heap_pages(
     }
     first_node = node;
 
-    ASSERT(node >= 0);
     ASSERT(zone_lo <= zone_hi);
     ASSERT(zone_hi < NR_ZONES);
 
@@ -812,7 +811,6 @@ static void free_heap_pages(
     unsigned int zone = page_to_zone(pg);
 
     ASSERT(order <= MAX_ORDER);
-    ASSERT(node >= 0);
 
     spin_lock(&heap_lock);
 
-- 
1.7.10.4

Coverity ID: 1055319

Coverity identified that when passed a microcode header with a length field of
0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL, data, 0)
which is undefined behaviour.

While Xen''s implementation of memcpy will do the correct thing in this
case,
any user trying to load a 0 length microcode blob deserves an -EINVAL.

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>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/microcode_amd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index a3ceef8..2c767a9 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -202,7 +202,7 @@ static int get_ucode_from_buffer_amd(
         return -EINVAL;
     }
 
-    if ( (off + mpbuf->len) > bufsize )
+    if ( mpbuf->len == 0 || ((off + mpbuf->len) > bufsize) )
     {
         printk(KERN_ERR "microcode: Bad data in microcode data
file\n");
         return -EINVAL;
-- 
1.7.10.4

At 13:10 +0100 on 24 Sep (1380028236), Andrew Cooper
wrote:
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -289,7 +289,6 @@ static void hap_free_p2m_page(struct domain *d, struct
page_info *pg)
>      d->arch.paging.hap.p2m_pages--;
>      d->arch.paging.hap.total_pages++;
>      hap_free(d, page_to_mfn(pg));
> -    ASSERT(d->arch.paging.hap.p2m_pages >= 0);
Huh, I''m surprised clang didn''t complain about that one.

Acked-by: Tim Deegan <tim@xen.org>.

>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> So NULL checks are logically dead code.
That''s the case currently, but will need to change sooner or later.
Hence I''d prefer to keep these...

Jan
> Coverity IDs: 1055247, 1087147
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/hvm/vmsi.c |    2 --
>  xen/arch/x86/irq.c      |    2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 4826b4a..b0766a4 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -304,8 +304,6 @@ static int msixtbl_write(struct vcpu *v, unsigned long 
> address,
>          goto out;
>      
>      desc = irq_to_desc(msi_desc->irq);
> -    if ( !desc )
> -        goto out;
>  
>      spin_lock_irqsave(&desc->lock, flags);
>  
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index c61cc46..e6036ec 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -646,8 +646,6 @@ void irq_move_cleanup_interrupt(struct cpu_user_regs 
> *regs)
>              continue;
>  
>          desc = irq_to_desc(irq);
> -        if (!desc)
> -            continue;
>  
>          spin_lock(&desc->lock);
>          if (!desc->arch.move_cleanup_count)
> -- 
> 1.7.10.4

>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity ID 1055622, 1055630
> 
> node is an unsigned integer, so neccesserily >= 0.  There does not
appear to
> be any purpose to these assertions in the first place.
I think assertions should be treated more forgivingly than "normal"
code: The check is very valid is the type of "node" got changed
back to a signed int, as "node" is being used as array index. It is
quite likely that on such type changes people would not remember
to add back respective assertions.

That said - I''m not really opposed to this change, I just think that
assertions like the one here have documentation aspects going
beyond mere correctness checking intentions.

Jan
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/common/page_alloc.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 41251b2..8cab81b 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -585,7 +585,6 @@ static struct page_info *alloc_heap_pages(
>      }
>      first_node = node;
>  
> -    ASSERT(node >= 0);
>      ASSERT(zone_lo <= zone_hi);
>      ASSERT(zone_hi < NR_ZONES);
>  
> @@ -812,7 +811,6 @@ static void free_heap_pages(
>      unsigned int zone = page_to_zone(pg);
>  
>      ASSERT(order <= MAX_ORDER);
> -    ASSERT(node >= 0);
>  
>      spin_lock(&heap_lock);
>  
> -- 
> 1.7.10.4

>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1055319
> 
> Coverity identified that when passed a microcode header with a length field
of
> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL, data, 0)
> which is undefined behaviour.
I think that''s at least questionable: memcpy(..., 0) can hardly be
anything but a no-op, no matter whether either of the two pointers
in fact is a NULL one.

That''s not to say that I disagree that strict reading of the C standard
may indeed yield this undefined, but I don''t think we are to hunt
down all such undefined-nesses. The tool should really stay away
from hair-splitting, and concentrate on pointing out real issues.

Jan
> While Xen''s implementation of memcpy will do the correct thing in
this case,
> any user trying to load a 0 length microcode blob deserves an -EINVAL.
> 
> 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>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/arch/x86/microcode_amd.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index a3ceef8..2c767a9 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -202,7 +202,7 @@ static int get_ucode_from_buffer_amd(
>          return -EINVAL;
>      }
>  
> -    if ( (off + mpbuf->len) > bufsize )
> +    if ( mpbuf->len == 0 || ((off + mpbuf->len) > bufsize) )
>      {
>          printk(KERN_ERR "microcode: Bad data in microcode data
file\n");
>          return -EINVAL;
> -- 
> 1.7.10.4

On 09/24/2013 09:19 AM, Jan Beulich wrote:
>>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> Coverity ID: 1055319
>>
>> Coverity identified that when passed a microcode header with a length
field of
>> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL, data,
0)
>> which is undefined behaviour.
> I think that''s at least questionable: memcpy(..., 0) can hardly be
> anything but a no-op, no matter whether either of the two pointers
> in fact is a NULL one.
I think what this patch is trying to prevent is passing NULL pointer to 
memcpy,
not length being zero (if you follow the logic in 
get_ucode_from_buffer_amd() you
will see that destination buffer may not be allocated when length is zero).

AMD APM says

   REP. The REP prefix repeats its associated string instruction the number
   of times specified in the counter register (rCX). It terminates the
   repetition when the value in rCX reaches 0.

So presumably rCX is checked before rDI and memcpy would indeed be a nop.

Still, I think in this particular case (destination can be NULL) the 
patch is
a good thing since we usually (often) check before passing NULL pointer to
routines. Alternatively, we can check ''if (mc_amd->mpb !=
NULL)''.

-boris
>
> That''s not to say that I disagree that strict reading of the C
standard
> may indeed yield this undefined, but I don''t think we are to hunt
> down all such undefined-nesses. The tool should really stay away
> from hair-splitting, and concentrate on pointing out real issues.
>
> Jan
>
>> While Xen''s implementation of memcpy will do the correct thing
in this case,
>> any user trying to load a 0 length microcode blob deserves an -EINVAL.
>>
>> 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>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   xen/arch/x86/microcode_amd.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/microcode_amd.c
b/xen/arch/x86/microcode_amd.c
>> index a3ceef8..2c767a9 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -202,7 +202,7 @@ static int get_ucode_from_buffer_amd(
>>           return -EINVAL;
>>       }
>>   
>> -    if ( (off + mpbuf->len) > bufsize )
>> +    if ( mpbuf->len == 0 || ((off + mpbuf->len) > bufsize) )
>>       {
>>           printk(KERN_ERR "microcode: Bad data in microcode data
file\n");
>>           return -EINVAL;
>> -- 
>> 1.7.10.4
>
>

>>> On 24.09.13 at 16:26, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 09/24/2013 09:19 AM, Jan Beulich wrote:
>>>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>> Coverity ID: 1055319
>>>
>>> Coverity identified that when passed a microcode header with a
length field
> of
>>> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL,
data, 0)
>>> which is undefined behaviour.
>> I think that''s at least questionable: memcpy(..., 0) can
hardly be
>> anything but a no-op, no matter whether either of the two pointers
>> in fact is a NULL one.
> 
> I think what this patch is trying to prevent is passing NULL pointer to 
> memcpy,
> not length being zero (if you follow the logic in 
> get_ucode_from_buffer_amd() you
> will see that destination buffer may not be allocated when length is zero).
I understand that; what I was trying to say is that it can''t be other
than benign if you pass NULL for at least one of the two pointers
as long as 0 is being passed for the size.

Jan

On 24/09/13 15:26, Boris Ostrovsky wrote:
> On 09/24/2013 09:19 AM, Jan Beulich wrote:
>>>>> On 24.09.13 at 14:10, Andrew Cooper
<andrew.cooper3@citrix.com>
>>>>> wrote:
>>> Coverity ID: 1055319
>>>
>>> Coverity identified that when passed a microcode header with a
>>> length field of
>>> 0, get_ucode_from_buffer_amd() would end up calling memcpy(NULL,
>>> data, 0)
>>> which is undefined behaviour.
>> I think that''s at least questionable: memcpy(..., 0) can
hardly be
>> anything but a no-op, no matter whether either of the two pointers
>> in fact is a NULL one.
>
> I think what this patch is trying to prevent is passing NULL pointer
> to memcpy,
> not length being zero (if you follow the logic in
> get_ucode_from_buffer_amd() you
> will see that destination buffer may not be allocated when length is
> zero).
>
> AMD APM says
>
>   REP. The REP prefix repeats its associated string instruction the
> number
>   of times specified in the counter register (rCX). It terminates the
>   repetition when the value in rCX reaches 0.
>
> So presumably rCX is checked before rDI and memcpy would indeed be a nop.
>
> Still, I think in this particular case (destination can be NULL) the
> patch is
> a good thing since we usually (often) check before passing NULL
> pointer to
> routines. Alternatively, we can check ''if (mc_amd->mpb !=
NULL)''.
>
> -boris
That may not work as expected.

The subtalty here is that we only allocate memory for mc_amd->mpb if
mc_amd->mpb_size is less than mpbuf->len.

mc_amd->mpb is unconditionally NULL before the first allocation, and
mc_amd->mpb_size is 0 (from the caller)

Therefore, a user passing 0 for mpbuf->len causes the routine to choose
not to allocate any memory, then copy 0 bytes into it.

~Andrew