Virtualization - Sep 2019 - [bug report] x86/paravirt: Use a single ops structure

Hello Juergen Gross,

The patch 5c83511bdb98: "x86/paravirt: Use a single ops structure"
from Aug 28, 2018, leads to the following static checker warning:

	arch/x86/kernel/paravirt.c:123 paravirt_patch_default()
	warn: uncapped user index '*(&pv_ops + type)'

	arch/x86/kernel/paravirt.c:124 paravirt_patch_default()
	error: buffer overflow '&pv_ops' 90 <= 255
user_rl='0-255'

arch/x86/kernel/paravirt.c
   116  unsigned paravirt_patch_default(u8 type, void *insn_buff,
   117                                  unsigned long addr, unsigned len)
   118  {
   119          /*
   120           * Neat trick to map patch type back to the call within the
   121           * corresponding structure.
   122           */
   123          void *opfunc = *((void **)&pv_ops + type);
                                          ^^^^^^^^^^^^^^
This code is actually pretty old...

This isn't a security issue, but the size of &pv_ops is variable but
type isn't checked so we could be reading beyond the end.  We could do
something like:

        if (type >= sizeof(pv_ops) / sizeof(void *))
                return -EINVAL;
	opfunc = *((void **)&pv_ops + type);

   124          unsigned ret;
   125  
   126          if (opfunc == NULL)
   127                  /* If there's no function, patch it with a ud2a
(BUG) */
   128                  ret = paravirt_patch_insns(insn_buff, len, ud2a,
ud2a+sizeof(ud2a));
   129          else if (opfunc == _paravirt_nop)
   130                  ret = 0;
   131  

regards,
dan carpenter

On 10.09.19 16:53, Dan Carpenter wrote:
> Hello Juergen Gross,
> 
> The patch 5c83511bdb98: "x86/paravirt: Use a single ops
structure"
> from Aug 28, 2018, leads to the following static checker warning:
> 
> 	arch/x86/kernel/paravirt.c:123 paravirt_patch_default()
> 	warn: uncapped user index '*(&pv_ops + type)'
> 
> 	arch/x86/kernel/paravirt.c:124 paravirt_patch_default()
> 	error: buffer overflow '&pv_ops' 90 <= 255
user_rl='0-255'
> 
> arch/x86/kernel/paravirt.c
>     116  unsigned paravirt_patch_default(u8 type, void *insn_buff,
>     117                                  unsigned long addr, unsigned len)
>     118  {
>     119          /*
>     120           * Neat trick to map patch type back to the call within
the
>     121           * corresponding structure.
>     122           */
>     123          void *opfunc = *((void **)&pv_ops + type);
>                                            ^^^^^^^^^^^^^^
> This code is actually pretty old...
> 
> This isn't a security issue, but the size of &pv_ops is variable
but
> type isn't checked so we could be reading beyond the end.  We could do
> something like:
> 
>          if (type >= sizeof(pv_ops) / sizeof(void *))
>                  return -EINVAL;
No, not really. Please check how th return value is being used: it
specifies the length of the instruction to patch. Just returning
-EINVAL would probably clobber most of the kernel.

Please note that type is derived from the pv_ops field names, so
in reality the risk to read beyond the end of pv_ops is zero.


Juergen