[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/pv: Sanity check bytes_per_rep in rep_outs()



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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.