| Message ID | 20240511151642.2476555-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pv: Sanity check bytes_per_rep in rep_outs() | expand |
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.
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.
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;
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(+)