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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
