Re: [bug report] x86/paravirt: Use a single ops structure
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 '*(_ops + type)'
arch/x86/kernel/paravirt.c:124 paravirt_patch_default()
error: buffer overflow '_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 **)_ops + type);
^^
This code is actually pretty old...
This isn't a security issue, but the size of _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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 '*(_ops + type)'
arch/x86/kernel/paravirt.c:124 paravirt_patch_default()
error: buffer overflow '_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 **)_ops + type);
^^
This code is actually pretty old...
This isn't a security issue, but the size of _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 **)_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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
