diff mbox

[1/3] xen/common: Avoid undefined behaviour by shifting into a sign bit

Message ID 1470405013-18856-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Aug. 5, 2016, 1:50 p.m. UTC
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
---
 xen/common/domctl.c       | 2 +-
 xen/common/xmalloc_tlsf.c | 4 ++--
 xen/include/xen/sched.h   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Jan Beulich Aug. 5, 2016, 2:04 p.m. UTC | #1
>>> On 05.08.16 at 15:50, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -188,7 +188,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>          (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
>          (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
>          (d->is_xenstore                 ? XEN_DOMINF_xs_domain : 0) |
> -        d->shutdown_code << XEN_DOMINF_shutdownshift;
> +        (unsigned int)d->shutdown_code << XEN_DOMINF_shutdownshift;

Is adding a cast here really the most suitable fix? The only two places
shutdown_code gets set (besides the -1 initialization) have their right
side a u8. Nothing ever checks for the value being negative (there are
just two -1 checks), so converting the field to u8 or unsigned int (and
using a sentinel different from -1) should both work, avoiding the need
for a cast.

Jan
George Dunlap Aug. 5, 2016, 2:07 p.m. UTC | #2
On 05/08/16 15:04, Jan Beulich wrote:
>>>> On 05.08.16 at 15:50, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -188,7 +188,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>          (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
>>          (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
>>          (d->is_xenstore                 ? XEN_DOMINF_xs_domain : 0) |
>> -        d->shutdown_code << XEN_DOMINF_shutdownshift;
>> +        (unsigned int)d->shutdown_code << XEN_DOMINF_shutdownshift;
> 
> Is adding a cast here really the most suitable fix? The only two places
> shutdown_code gets set (besides the -1 initialization) have their right
> side a u8. Nothing ever checks for the value being negative (there are
> just two -1 checks), so converting the field to u8 or unsigned int (and
> using a sentinel different from -1) should both work, avoiding the need
> for a cast.

This seems sensible if possible.

The other bits:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
diff mbox

Patch

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 8f25131..cf7928c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -188,7 +188,7 @@  void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
         (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
         (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
         (d->is_xenstore                 ? XEN_DOMINF_xs_domain : 0) |
-        d->shutdown_code << XEN_DOMINF_shutdownshift;
+        (unsigned int)d->shutdown_code << XEN_DOMINF_shutdownshift;
 
     switch ( d->guest_type )
     {
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index b13317e..6c1b882 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -177,7 +177,7 @@  static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl)
 static inline struct bhdr *FIND_SUITABLE_BLOCK(struct xmem_pool *p, int *fl,
                                                int *sl)
 {
-    u32 tmp = p->sl_bitmap[*fl] & (~0 << *sl);
+    u32 tmp = p->sl_bitmap[*fl] & (~0u << *sl);
     struct bhdr *b = NULL;
 
     if ( tmp )
@@ -187,7 +187,7 @@  static inline struct bhdr *FIND_SUITABLE_BLOCK(struct xmem_pool *p, int *fl,
     }
     else
     {
-        *fl = ffs(p->fl_bitmap & (~0 << (*fl + 1))) - 1;
+        *fl = ffs(p->fl_bitmap & (~0u << (*fl + 1))) - 1;
         if ( likely(*fl > 0) )
         {
             *sl = ffs(p->sl_bitmap[*fl]) - 1;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 888bc19..bb4ee4e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -483,7 +483,7 @@  extern struct vcpu *idle_vcpu[NR_CPUS];
 #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
 #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
 
-#define DOMAIN_DESTROYED (1<<31) /* assumes atomic_t is >= 32 bits */
+#define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits */
 #define put_domain(_d) \
   if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)