[Xen-devel] [PATCH] x86/pv: Fix undefined behaviour in check_descriptor()

Andrew Cooper posted 1 patch 4 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/1559832777-7818-1-git-send-email-andrew.cooper3@citrix.com
xen/arch/x86/x86_64/mm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[Xen-devel] [PATCH] x86/pv: Fix undefined behaviour in check_descriptor()
Posted by Andrew Cooper 4 years, 10 months ago
UBSAN reports:

  (XEN) ================================================================================
  (XEN) UBSAN: Undefined behaviour in x86_64/mm.c:1108:31
  (XEN) left shift of 255 by 24 places cannot be represented in type 'int'
  (XEN) ----[ Xen-4.13-unstable  x86_64  debug=y   Tainted:    H ]----
  (XEN) CPU:    60
  (XEN) RIP:    e008:[<ffff82d0802a54ce>] ubsan.c#ubsan_epilogue+0xa/0xc2
  <snip>
  (XEN) Xen call trace:
  (XEN)    [<ffff82d0802a54ce>] ubsan.c#ubsan_epilogue+0xa/0xc2
  (XEN)    [<ffff82d0802a6009>] __ubsan_handle_shift_out_of_bounds+0x15d/0x16c
  (XEN)    [<ffff82d08033abd7>] check_descriptor+0x191/0x3dd
  (XEN)    [<ffff82d0804ef920>] do_update_descriptor+0x7f/0x2b6
  (XEN)    [<ffff82d0804efb75>] compat_update_descriptor+0x1e/0x20
  (XEN)    [<ffff82d0804fa1cc>] pv_hypercall+0x87f/0xa6f
  (XEN)    [<ffff82d080501acb>] do_entry_int82+0x53/0x58
  (XEN)    [<ffff82d08050702b>] entry_int82+0xbb/0xc0
  (XEN)
  (XEN) ================================================================================

Switch to an unsigned shift.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/x86_64/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d8f558b..1721dcb 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1105,7 +1105,7 @@ int check_descriptor(const struct domain *dom, seg_desc_t *d)
              * 0xf6800000. Extend these to allow access to the larger read-only
              * M2P table available in 32on64 mode.
              */
-            base = (b & (0xff << 24)) | ((b & 0xff) << 16) | (a >> 16);
+            base = (b & (0xffu << 24)) | ((b & 0xff) << 16) | (a >> 16);
 
             limit = (b & 0xf0000) | (a & 0xffff);
             limit++; /* We add one because limit is inclusive. */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/pv: Fix undefined behaviour in check_descriptor()
Posted by Jan Beulich 4 years, 10 months ago
>>> On 06.06.19 at 16:52, <andrew.cooper3@citrix.com> wrote:
> UBSAN reports:
> 
>   (XEN) 
> =============================================================================
> ===
>   (XEN) UBSAN: Undefined behaviour in x86_64/mm.c:1108:31
>   (XEN) left shift of 255 by 24 places cannot be represented in type 'int'
>   (XEN) ----[ Xen-4.13-unstable  x86_64  debug=y   Tainted:    H ]----
>   (XEN) CPU:    60
>   (XEN) RIP:    e008:[<ffff82d0802a54ce>] ubsan.c#ubsan_epilogue+0xa/0xc2
>   <snip>
>   (XEN) Xen call trace:
>   (XEN)    [<ffff82d0802a54ce>] ubsan.c#ubsan_epilogue+0xa/0xc2
>   (XEN)    [<ffff82d0802a6009>] __ubsan_handle_shift_out_of_bounds+0x15d/0x16c
>   (XEN)    [<ffff82d08033abd7>] check_descriptor+0x191/0x3dd
>   (XEN)    [<ffff82d0804ef920>] do_update_descriptor+0x7f/0x2b6
>   (XEN)    [<ffff82d0804efb75>] compat_update_descriptor+0x1e/0x20
>   (XEN)    [<ffff82d0804fa1cc>] pv_hypercall+0x87f/0xa6f
>   (XEN)    [<ffff82d080501acb>] do_entry_int82+0x53/0x58
>   (XEN)    [<ffff82d08050702b>] entry_int82+0xbb/0xc0
>   (XEN)
>   (XEN) 
> =============================================================================
> ===
> 
> Switch to an unsigned shift.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1105,7 +1105,7 @@ int check_descriptor(const struct domain *dom, seg_desc_t *d)
>               * 0xf6800000. Extend these to allow access to the larger read-only
>               * M2P table available in 32on64 mode.
>               */
> -            base = (b & (0xff << 24)) | ((b & 0xff) << 16) | (a >> 16);
> +            base = (b & (0xffu << 24)) | ((b & 0xff) << 16) | (a >> 16);
>  
>              limit = (b & 0xf0000) | (a & 0xffff);

Seeing this the other one could as well use (b & 0xff000000).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel