* [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs()
@ 2024-05-11 15:16 Andrew Cooper
2024-05-14 7:05 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2024-05-11 15:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
I was playing with GCC-14, and gave the new static analyser (-fanalyze) a
try. One of the more serious things it came back with was this:
In file included from ./arch/x86/include/asm/mm.h:10,
from ./include/xen/mm.h:233,
from ./include/xen/domain_page.h:12,
from arch/x86/pv/emul-priv-op.c:10:
In function '__copy_from_guest_pv',
inlined from 'rep_outs' at arch/x86/pv/emul-priv-op.c:718:20:
./arch/x86/include/asm/uaccess.h:174:9: warning: stack-based buffer overflow [CWE-121] [-Wanalyzer-out-of-bounds]
174 | __asm__ __volatile__( \
| ^~~~~~~
./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
| ^~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
| ^~~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
| ^~~~~~~~~~~~~~
'rep_outs': events 1-3
|
|arch/x86/pv/emul-priv-op.c:674:21:
| 674 | static int cf_check rep_outs(
| | ^~~~~~~~
| | |
| | (1) entry to 'rep_outs'
|......
| 688 | if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) )
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (3) calling 'guest_io_okay' from 'rep_outs'
|......
| 710 | unsigned int data = 0;
| | ~~~~
| | |
| | (2) capacity: 4 bytes
|
+--> 'guest_io_okay': events 4-5
|
| 159 | static bool guest_io_okay(unsigned int port, unsigned int bytes,
| | ^~~~~~~~~~~~~
| | |
| | (4) entry to 'guest_io_okay'
|......
| 165 | if ( iopl_ok(v, regs) )
| | ~~~~~~~~~~~~~~~~
| | |
| | (5) calling 'iopl_ok' from 'guest_io_okay'
|
+--> 'iopl_ok': event 6
|
| 148 | static bool iopl_ok(const struct vcpu *v, const struct cpu_user_regs *regs)
| | ^~~~~~~
| | |
| | (6) entry to 'iopl_ok'
|
'iopl_ok': event 7
|
|./include/xen/bug.h:141:13:
| 141 | do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
| | ^
| | |
| | (7) following 'false' branch...
arch/x86/pv/emul-priv-op.c:153:5: note: in expansion of macro 'ASSERT'
| 153 | ASSERT((v->arch.pv.iopl & ~X86_EFLAGS_IOPL) == 0);
| | ^~~~~~
|
'iopl_ok': event 8
|
|./include/xen/bug.h:124:31:
| 124 | #define assert_failed(msg) do { \
| | ^
| | |
| | (8) ...to here
./include/xen/bug.h:141:32: note: in expansion of macro 'assert_failed'
| 141 | do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
| | ^~~~~~~~~~~~~
arch/x86/pv/emul-priv-op.c:153:5: note: in expansion of macro 'ASSERT'
| 153 | ASSERT((v->arch.pv.iopl & ~X86_EFLAGS_IOPL) == 0);
| | ^~~~~~
|
<------+
|
'guest_io_okay': event 9
|
| 165 | if ( iopl_ok(v, regs) )
| | ^~~~~~~~~~~~~~~~
| | |
| | (9) returning to 'guest_io_okay' from 'iopl_ok'
|
<------+
|
'rep_outs': events 10-13
|
| 688 | if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) )
| | ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | |
| | | (10) returning to 'rep_outs' from 'guest_io_okay'
| | (11) following 'true' branch...
|......
| 691 | rc = read_segment(seg, &sreg, ctxt);
| | ~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | |
| | | (13) calling 'read_segment' from 'rep_outs'
| | (12) ...to here
|
+--> 'read_segment': events 14-18
|
| 493 | static int cf_check read_segment(
| | ^~~~~~~~~~~~
| | |
| | (14) entry to 'read_segment'
|......
| 510 | if ( ctxt->addr_size < 64 )
| | ~
| | |
| | (15) following 'false' branch...
|......
| 535 | switch ( seg )
| | ~~~~~~
| | |
| | (16) ...to here
|......
| 538 | if ( !is_x86_user_segment(seg) )
| | ~
| | |
| | (17) following 'false' branch (when 'seg <= 5')...
| 539 | return X86EMUL_UNHANDLEABLE;
| 540 | reg->base = 0;
| | ~~~
| | |
| | (18) ...to here
|
<------+
|
'rep_outs': events 19-30
|
| 691 | rc = read_segment(seg, &sreg, ctxt);
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (19) returning to 'rep_outs' from 'read_segment'
| 692 | if ( rc != X86EMUL_OKAY )
| | ~
| | |
| | (20) following 'false' branch (when 'rc == 0')...
|......
| 695 | if ( !sreg.p )
| | ~~ ~
| | | |
| | | (22) following 'false' branch...
| | (21) ...to here
| 696 | return X86EMUL_UNHANDLEABLE;
| 697 | if ( !sreg.s ||
| | ~~ ~ ~~~~~~~~~~
| | | | |
| | | | (26) following 'false' branch...
| | | (24) following 'false' branch...
| | (23) ...to here
| 698 | ((sreg.type & (_SEGMENT_CODE >> 8)) &&
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (25) ...to here
| 699 | !(sreg.type & (_SEGMENT_WR >> 8))) )
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|......
| 706 | poc->bpmatch = check_guest_io_breakpoint(curr, port, bytes_per_rep);
| | ~~~
| | |
| | (27) ...to here
| 707 |
| 708 | while ( *reps < goal )
| | ~~~~~~~~~~~~
| | |
| | (28) following 'true' branch...
| 709 | {
| 710 | unsigned int data = 0;
| | ~~~~~~~~
| | |
| | (29) ...to here
|......
| 713 | rc = pv_emul_virt_to_linear(sreg.base, offset, bytes_per_rep,
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (30) calling 'pv_emul_virt_to_linear' from 'rep_outs'
| 714 | sreg.limit, seg, ctxt, &addr);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
+--> 'pv_emul_virt_to_linear': events 31-33
|
| 580 | static int pv_emul_virt_to_linear(unsigned long base, unsigned long offset,
| | ^~~~~~~~~~~~~~~~~~~~~~
| | |
| | (31) entry to 'pv_emul_virt_to_linear'
|......
| 596 | else if ( !__addr_ok(*addr) )
| | ~
| | |
| | (32) following 'false' branch...
|......
| 603 | return rc;
| | ~~~~~~
| | |
| | (33) ...to here
|
<------+
|
'rep_outs': events 34-37
|
| 713 | rc = pv_emul_virt_to_linear(sreg.base, offset, bytes_per_rep,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (34) returning to 'rep_outs' from 'pv_emul_virt_to_linear'
| 714 | sreg.limit, seg, ctxt, &addr);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 715 | if ( rc != X86EMUL_OKAY )
| | ~
| | |
| | (35) following 'false' branch (when 'rc == 0')...
|......
| 718 | if ( (rc = __copy_from_guest_pv(&data, (void __user *)addr,
| | ~~ ~
| | | |
| | | (37) inlined call to '__copy_from_guest_pv' from 'rep_outs'
| | (36) ...to here
|
+--> '__copy_from_guest_pv': events 38-41
|
|./arch/x86/include/asm/uaccess.h:294:8:
| 294 | if (__builtin_constant_p(n)) {
| | ^
| | |
| | (38) following 'true' branch...
| 295 | unsigned long ret;
| | ~~~~~~~~
| | |
| | (39) ...to here
| 296 |
| 297 | switch (n) {
| | ~~~~~~
| | |
| | (40) following 'case 8:' branch...
|......
| 307 | case 8:
| | ~~~~
| | |
| | (41) ...to here
|
'__copy_from_guest_pv': event 42
|
| 174 | __asm__ __volatile__( \
| | ^~~~~~~
| | |
| | (42) out-of-bounds write from byte 4 till byte 7 but 'data' ends at byte 4
./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
| 229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
| | ^~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
| 236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
| | ^~~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
| 308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
| | ^~~~~~~~~~~~~~
|
./arch/x86/include/asm/uaccess.h:174:9: note: write of 4 bytes to beyond the end of 'data'
174 | __asm__ __volatile__( \
| ^~~~~~~
./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
| ^~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
| ^~~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
| ^~~~~~~~~~~~~~
┌──────────────────────────────────────────────────────────────────────┐
│ write of 'uint64_t' (8 bytes) │
└──────────────────────────────────────────────────────────────────────┘
│ │
│ │
v v
┌──────────────────────────────────┐┌──────────────────────────────────┐
│ 'data' (type: 'unsigned int') ││ after valid range │
└──────────────────────────────────┘└──────────────────────────────────┘
├────────────────┬─────────────────┤├────────────────┬─────────────────┤
│ │
╭────────┴────────╮ ╭───────────┴──────────╮
│capacity: 4 bytes│ │⚠️ overflow of 4 bytes│
╰─────────────────╯ ╰──────────────────────╯
What is happening is that it's seen that __copy_from_guest_pv() has a case for
8, and it doesn't have any information about bytes_per_rep which is an input
to the function.
Architecturally, there's no 64-bit variant of OUTS in x86. Leaving an
assertion to this effect is enough to satisfy the analyser.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
A little RFC, but the analyser is right - if rep_outs() were to be called with
anything more than 4, the stack would be clobbered, and no developer is
finding that bug the easy way...
---
xen/arch/x86/pv/emul-priv-op.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index f101510a1bab..1460ecc187be 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -685,6 +685,8 @@ static int cf_check rep_outs(
*reps = 0;
+ ASSERT(bytes_per_rep <= 4); /* i.e. 'data' being 4 bytes is fine. */
+
if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) )
return X86EMUL_UNHANDLEABLE;
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs()
2024-05-11 15:16 [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs() Andrew Cooper
@ 2024-05-14 7:05 ` Roger Pau Monné
2024-05-14 7:22 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monné @ 2024-05-14 7:05 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich
On Sat, May 11, 2024 at 04:16:42PM +0100, Andrew Cooper wrote:
> I was playing with GCC-14, and gave the new static analyser (-fanalyze) a
> try. One of the more serious things it came back with was this:
>
> In file included from ./arch/x86/include/asm/mm.h:10,
> from ./include/xen/mm.h:233,
> from ./include/xen/domain_page.h:12,
> from arch/x86/pv/emul-priv-op.c:10:
> In function '__copy_from_guest_pv',
> inlined from 'rep_outs' at arch/x86/pv/emul-priv-op.c:718:20:
> ./arch/x86/include/asm/uaccess.h:174:9: warning: stack-based buffer overflow [CWE-121] [-Wanalyzer-out-of-bounds]
> 174 | __asm__ __volatile__( \
> | ^~~~~~~
> ./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
> 229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
> | ^~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
> 236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
> | ^~~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
> 308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
> | ^~~~~~~~~~~~~~
> 'rep_outs': events 1-3
> |
> |arch/x86/pv/emul-priv-op.c:674:21:
> | 674 | static int cf_check rep_outs(
> | | ^~~~~~~~
> | | |
> | | (1) entry to 'rep_outs'
> |......
> | 688 | if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) )
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (3) calling 'guest_io_okay' from 'rep_outs'
> |......
> | 710 | unsigned int data = 0;
> | | ~~~~
> | | |
> | | (2) capacity: 4 bytes
> |
> +--> 'guest_io_okay': events 4-5
> |
> | 159 | static bool guest_io_okay(unsigned int port, unsigned int bytes,
> | | ^~~~~~~~~~~~~
> | | |
> | | (4) entry to 'guest_io_okay'
> |......
> | 165 | if ( iopl_ok(v, regs) )
> | | ~~~~~~~~~~~~~~~~
> | | |
> | | (5) calling 'iopl_ok' from 'guest_io_okay'
> |
> +--> 'iopl_ok': event 6
> |
> | 148 | static bool iopl_ok(const struct vcpu *v, const struct cpu_user_regs *regs)
> | | ^~~~~~~
> | | |
> | | (6) entry to 'iopl_ok'
> |
> 'iopl_ok': event 7
> |
> |./include/xen/bug.h:141:13:
> | 141 | do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> | | ^
> | | |
> | | (7) following 'false' branch...
> arch/x86/pv/emul-priv-op.c:153:5: note: in expansion of macro 'ASSERT'
> | 153 | ASSERT((v->arch.pv.iopl & ~X86_EFLAGS_IOPL) == 0);
> | | ^~~~~~
> |
> 'iopl_ok': event 8
> |
> |./include/xen/bug.h:124:31:
> | 124 | #define assert_failed(msg) do { \
> | | ^
> | | |
> | | (8) ...to here
> ./include/xen/bug.h:141:32: note: in expansion of macro 'assert_failed'
> | 141 | do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> | | ^~~~~~~~~~~~~
> arch/x86/pv/emul-priv-op.c:153:5: note: in expansion of macro 'ASSERT'
> | 153 | ASSERT((v->arch.pv.iopl & ~X86_EFLAGS_IOPL) == 0);
> | | ^~~~~~
> |
> <------+
> |
> 'guest_io_okay': event 9
> |
> | 165 | if ( iopl_ok(v, regs) )
> | | ^~~~~~~~~~~~~~~~
> | | |
> | | (9) returning to 'guest_io_okay' from 'iopl_ok'
> |
> <------+
> |
> 'rep_outs': events 10-13
> |
> | 688 | if ( !guest_io_okay(port, bytes_per_rep, curr, ctxt->regs) )
> | | ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | | |
> | | | (10) returning to 'rep_outs' from 'guest_io_okay'
> | | (11) following 'true' branch...
> |......
> | 691 | rc = read_segment(seg, &sreg, ctxt);
> | | ~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | | |
> | | | (13) calling 'read_segment' from 'rep_outs'
> | | (12) ...to here
> |
> +--> 'read_segment': events 14-18
> |
> | 493 | static int cf_check read_segment(
> | | ^~~~~~~~~~~~
> | | |
> | | (14) entry to 'read_segment'
> |......
> | 510 | if ( ctxt->addr_size < 64 )
> | | ~
> | | |
> | | (15) following 'false' branch...
> |......
> | 535 | switch ( seg )
> | | ~~~~~~
> | | |
> | | (16) ...to here
> |......
> | 538 | if ( !is_x86_user_segment(seg) )
> | | ~
> | | |
> | | (17) following 'false' branch (when 'seg <= 5')...
> | 539 | return X86EMUL_UNHANDLEABLE;
> | 540 | reg->base = 0;
> | | ~~~
> | | |
> | | (18) ...to here
> |
> <------+
> |
> 'rep_outs': events 19-30
> |
> | 691 | rc = read_segment(seg, &sreg, ctxt);
> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (19) returning to 'rep_outs' from 'read_segment'
> | 692 | if ( rc != X86EMUL_OKAY )
> | | ~
> | | |
> | | (20) following 'false' branch (when 'rc == 0')...
> |......
> | 695 | if ( !sreg.p )
> | | ~~ ~
> | | | |
> | | | (22) following 'false' branch...
> | | (21) ...to here
> | 696 | return X86EMUL_UNHANDLEABLE;
> | 697 | if ( !sreg.s ||
> | | ~~ ~ ~~~~~~~~~~
> | | | | |
> | | | | (26) following 'false' branch...
> | | | (24) following 'false' branch...
> | | (23) ...to here
> | 698 | ((sreg.type & (_SEGMENT_CODE >> 8)) &&
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (25) ...to here
> | 699 | !(sreg.type & (_SEGMENT_WR >> 8))) )
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> |......
> | 706 | poc->bpmatch = check_guest_io_breakpoint(curr, port, bytes_per_rep);
> | | ~~~
> | | |
> | | (27) ...to here
> | 707 |
> | 708 | while ( *reps < goal )
> | | ~~~~~~~~~~~~
> | | |
> | | (28) following 'true' branch...
> | 709 | {
> | 710 | unsigned int data = 0;
> | | ~~~~~~~~
> | | |
> | | (29) ...to here
> |......
> | 713 | rc = pv_emul_virt_to_linear(sreg.base, offset, bytes_per_rep,
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (30) calling 'pv_emul_virt_to_linear' from 'rep_outs'
> | 714 | sreg.limit, seg, ctxt, &addr);
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> |
> +--> 'pv_emul_virt_to_linear': events 31-33
> |
> | 580 | static int pv_emul_virt_to_linear(unsigned long base, unsigned long offset,
> | | ^~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (31) entry to 'pv_emul_virt_to_linear'
> |......
> | 596 | else if ( !__addr_ok(*addr) )
> | | ~
> | | |
> | | (32) following 'false' branch...
> |......
> | 603 | return rc;
> | | ~~~~~~
> | | |
> | | (33) ...to here
> |
> <------+
> |
> 'rep_outs': events 34-37
> |
> | 713 | rc = pv_emul_virt_to_linear(sreg.base, offset, bytes_per_rep,
> | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | | |
> | | (34) returning to 'rep_outs' from 'pv_emul_virt_to_linear'
> | 714 | sreg.limit, seg, ctxt, &addr);
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | 715 | if ( rc != X86EMUL_OKAY )
> | | ~
> | | |
> | | (35) following 'false' branch (when 'rc == 0')...
> |......
> | 718 | if ( (rc = __copy_from_guest_pv(&data, (void __user *)addr,
> | | ~~ ~
> | | | |
> | | | (37) inlined call to '__copy_from_guest_pv' from 'rep_outs'
> | | (36) ...to here
> |
> +--> '__copy_from_guest_pv': events 38-41
> |
> |./arch/x86/include/asm/uaccess.h:294:8:
> | 294 | if (__builtin_constant_p(n)) {
> | | ^
> | | |
> | | (38) following 'true' branch...
> | 295 | unsigned long ret;
> | | ~~~~~~~~
> | | |
> | | (39) ...to here
> | 296 |
> | 297 | switch (n) {
> | | ~~~~~~
> | | |
> | | (40) following 'case 8:' branch...
> |......
> | 307 | case 8:
> | | ~~~~
> | | |
> | | (41) ...to here
> |
> '__copy_from_guest_pv': event 42
> |
> | 174 | __asm__ __volatile__( \
> | | ^~~~~~~
> | | |
> | | (42) out-of-bounds write from byte 4 till byte 7 but 'data' ends at byte 4
> ./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
> | 229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
> | | ^~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
> | 236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
> | | ^~~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
> | 308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
> | | ^~~~~~~~~~~~~~
> |
> ./arch/x86/include/asm/uaccess.h:174:9: note: write of 4 bytes to beyond the end of 'data'
> 174 | __asm__ __volatile__( \
> | ^~~~~~~
> ./arch/x86/include/asm/uaccess.h:229:13: note: in expansion of macro 'get_unsafe_asm'
> 229 | case 8: get_unsafe_asm(x, ptr, grd, retval, "", "=r", errret); break; \
> | ^~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:236:5: note: in expansion of macro 'get_unsafe_size'
> 236 | get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
> | ^~~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:308:13: note: in expansion of macro 'get_guest_size'
> 308 | get_guest_size(*(uint64_t *)to, from, 8, ret, 8);
> | ^~~~~~~~~~~~~~
>
> ┌──────────────────────────────────────────────────────────────────────┐
> │ write of 'uint64_t' (8 bytes) │
> └──────────────────────────────────────────────────────────────────────┘
> │ │
> │ │
> v v
> ┌──────────────────────────────────┐┌──────────────────────────────────┐
> │ 'data' (type: 'unsigned int') ││ after valid range │
> └──────────────────────────────────┘└──────────────────────────────────┘
> ├────────────────┬─────────────────┤├────────────────┬─────────────────┤
> │ │
> ╭────────┴────────╮ ╭───────────┴──────────╮
> │capacity: 4 bytes│ │⚠️ overflow of 4 bytes│
> ╰─────────────────╯ ╰──────────────────────╯
>
> What is happening is that it's seen that __copy_from_guest_pv() has a case for
> 8, and it doesn't have any information about bytes_per_rep which is an input
> to the function.
>
> Architecturally, there's no 64-bit variant of OUTS in x86. Leaving an
> assertion to this effect is enough to satisfy the analyser.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> A little RFC, but the analyser is right - if rep_outs() were to be called with
> anything more than 4, the stack would be clobbered, and no developer is
> finding that bug the easy way...
> ---
> xen/arch/x86/pv/emul-priv-op.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index f101510a1bab..1460ecc187be 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -685,6 +685,8 @@ static int cf_check rep_outs(
>
> *reps = 0;
>
> + ASSERT(bytes_per_rep <= 4); /* i.e. 'data' being 4 bytes is fine. */
Don't we need this to be a BUG_ON() to satisfy the compiler also on
non-debug builds?
Or maybe:
if ( bytes_per_rep > 4 )
{
ASSERT_UNREACHABLE();
return X86EMUL_UNHANDLEABLE;
}
Would it be possible to add the check to guest_io_okay() itself?
Thanks, Roger.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs()
2024-05-14 7:05 ` Roger Pau Monné
@ 2024-05-14 7:22 ` Jan Beulich
0 siblings, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2024-05-14 7:22 UTC (permalink / raw)
To: Roger Pau Monné, Andrew Cooper; +Cc: Xen-devel
On 14.05.2024 09:05, Roger Pau Monné wrote:
> On Sat, May 11, 2024 at 04:16:42PM +0100, Andrew Cooper wrote:
>> Architecturally, there's no 64-bit variant of OUTS in x86.
This is the reason why personally ...
>> Leaving an
>> assertion to this effect is enough to satisfy the analyser.
... I view adding such, in particular ...
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -685,6 +685,8 @@ static int cf_check rep_outs(
>>
>> *reps = 0;
>>
>> + ASSERT(bytes_per_rep <= 4); /* i.e. 'data' being 4 bytes is fine. */
>
> Don't we need this to be a BUG_ON() to satisfy the compiler also on
> non-debug builds?
>
> Or maybe:
>
> if ( bytes_per_rep > 4 )
> {
> ASSERT_UNREACHABLE();
> return X86EMUL_UNHANDLEABLE;
> }
... such a non-debug-build-covering form as dead code. With suitable
information to hand, I guess Eclair / Misra might even say so, too.
Instead I view the analyzer report here as a weakness of how analysis
is (and has to be) done.
Jan
> Would it be possible to add the check to guest_io_okay() itself?
>
> Thanks, Roger.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-14 7:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-11 15:16 [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs() Andrew Cooper
2024-05-14 7:05 ` Roger Pau Monné
2024-05-14 7:22 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.