* [BUG] arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’: error: use of NULL ‘data’ where non-null expected
@ 2024-07-19 18:22 Mirsad Todorovac
2024-07-19 19:01 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Mirsad Todorovac @ 2024-07-19 18:22 UTC (permalink / raw)
To: kvm
Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel,
David Edmondson
Hi, all!
On linux-stable 6.10 vanilla tree, another NULL pointer is passed, which was detected
by the fortify-string.h mechanism.
arch/x86/kvm/x86.c
==================
13667 kvm_prepare_emulation_failure_exit(vcpu);
calls
8796 __kvm_prepare_emulation_failure_exit(vcpu, NULL, 0);
which calls
8790 prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
Note here that data == NULL and ndata = 0.
again data == NULL and ndata == 0, which passes unchanged all until
8773 memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data, ndata * sizeof(data[0]));
The problem code was introduced with the commit e615e355894e6.
arch/x86/kvm/x86.c
==================
8728 static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, u64 *data,
8729 u8 ndata, u8 *insn_bytes, u8 insn_size)
8730 {
8731 struct kvm_run *run = vcpu->run;
8732 u64 info[5];
8733 u8 info_start;
8734
8735 /*
8736 * Zero the whole array used to retrieve the exit info, as casting to
8737 * u32 for select entries will leave some chunks uninitialized.
8738 */
8739 memset(&info, 0, sizeof(info));
8740
8741 static_call(kvm_x86_get_exit_info)(vcpu, (u32 *)&info[0], &info[1],
8742 &info[2], (u32 *)&info[3],
8743 (u32 *)&info[4]);
8744
8745 run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
8746 run->emulation_failure.suberror = KVM_INTERNAL_ERROR_EMULATION;
8747
8748 /*
8749 * There's currently space for 13 entries, but 5 are used for the exit
8750 * reason and info. Restrict to 4 to reduce the maintenance burden
8751 * when expanding kvm_run.emulation_failure in the future.
8752 */
8753 if (WARN_ON_ONCE(ndata > 4))
8754 ndata = 4;
8755
8756 /* Always include the flags as a 'data' entry. */
8757 info_start = 1;
8758 run->emulation_failure.flags = 0;
8759
8760 if (insn_size) {
8761 BUILD_BUG_ON((sizeof(run->emulation_failure.insn_size) +
8762 sizeof(run->emulation_failure.insn_bytes) != 16));
8763 info_start += 2;
8764 run->emulation_failure.flags |=
8765 KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
8766 run->emulation_failure.insn_size = insn_size;
8767 memset(run->emulation_failure.insn_bytes, 0x90,
8768 sizeof(run->emulation_failure.insn_bytes));
8769 memcpy(run->emulation_failure.insn_bytes, insn_bytes, insn_size);
8770 }
8771
8772 memcpy(&run->internal.data[info_start], info, sizeof(info));
8773 memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data,
8774 ndata * sizeof(data[0]));
8775
8776 run->emulation_failure.ndata = info_start + ARRAY_SIZE(info) + ndata;
8777 }
8778
8779 static void prepare_emulation_ctxt_failure_exit(struct kvm_vcpu *vcpu)
8780 {
8781 struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
8782
8783 prepare_emulation_failure_exit(vcpu, NULL, 0, ctxt->fetch.data,
8784 ctxt->fetch.end - ctxt->fetch.data);
8785 }
8786
8787 void __kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, u64 *data,
8788 u8 ndata)
8789 {
8790 prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
8791 }
8792 EXPORT_SYMBOL_GPL(__kvm_prepare_emulation_failure_exit);
8793
8794 void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
8795 {
8796 __kvm_prepare_emulation_failure_exit(vcpu, NULL, 0);
8797 }
Probably before memcpy() with ndata == 0 ended in a NOOP, but now CONFIG_FORTIFY_SOURCE=y
turns a warning, or prevents the build with -Werror.
Hope this helps.
Best regards,
Mirsad Todorovac
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [BUG] arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’: error: use of NULL ‘data’ where non-null expected
2024-07-19 18:22 [BUG] arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’: error: use of NULL ‘data’ where non-null expected Mirsad Todorovac
@ 2024-07-19 19:01 ` Sean Christopherson
2024-07-19 19:24 ` Mirsad Todorovac
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2024-07-19 19:01 UTC (permalink / raw)
To: Mirsad Todorovac
Cc: kvm, Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, linux-kernel, David Edmondson
On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
> Hi, all!
>
> On linux-stable 6.10 vanilla tree, another NULL pointer is passed, which was detected
> by the fortify-string.h mechanism.
>
> arch/x86/kvm/x86.c
> ==================
>
> 13667 kvm_prepare_emulation_failure_exit(vcpu);
>
> calls
>
> 8796 __kvm_prepare_emulation_failure_exit(vcpu, NULL, 0);
>
> which calls
>
> 8790 prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
>
> Note here that data == NULL and ndata = 0.
>
> again data == NULL and ndata == 0, which passes unchanged all until
>
> 8773 memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data, ndata * sizeof(data[0]));
My reading of the C99 is that KVM's behavior is fine.
Where an argument declared as size_t n specifies the length of the array for a
function, n can have the value zero on a call to that function. Unless explicitly stated
otherwise in the description of a particular function in this subclause, pointer arguments
on such a call shall still have valid values, as described in 7.1.4. On such a call, a
function that locates a character finds no occurrence, a function that compares two
character sequences returns zero, and a function that copies characters copies zero
characters.
If the function copies zero characters, then there can't be a store to the NULL
pointer, and if there's no store, there's no NULL pointer explosion.
I suppose arguably one could argue the builtin memcpy() could deliberately fail
on an invalid pointer, but that'd be rather ridiculous.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [BUG] arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’: error: use of NULL ‘data’ where non-null expected
2024-07-19 19:01 ` Sean Christopherson
@ 2024-07-19 19:24 ` Mirsad Todorovac
0 siblings, 0 replies; 3+ messages in thread
From: Mirsad Todorovac @ 2024-07-19 19:24 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, linux-kernel, David Edmondson
On 7/19/24 21:01, Sean Christopherson wrote:
> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>> Hi, all!
>>
>> On linux-stable 6.10 vanilla tree, another NULL pointer is passed, which was detected
>> by the fortify-string.h mechanism.
>>
>> arch/x86/kvm/x86.c
>> ==================
>>
>> 13667 kvm_prepare_emulation_failure_exit(vcpu);
>>
>> calls
>>
>> 8796 __kvm_prepare_emulation_failure_exit(vcpu, NULL, 0);
>>
>> which calls
>>
>> 8790 prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
>>
>> Note here that data == NULL and ndata = 0.
>>
>> again data == NULL and ndata == 0, which passes unchanged all until
>>
>> 8773 memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data, ndata * sizeof(data[0]));
>
> My reading of the C99 is that KVM's behavior is fine.
>
> Where an argument declared as size_t n specifies the length of the array for a
> function, n can have the value zero on a call to that function. Unless explicitly stated
> otherwise in the description of a particular function in this subclause, pointer arguments
> on such a call shall still have valid values, as described in 7.1.4. On such a call, a
> function that locates a character finds no occurrence, a function that compares two
> character sequences returns zero, and a function that copies characters copies zero
> characters.
>
> If the function copies zero characters, then there can't be a store to the NULL
> pointer, and if there's no store, there's no NULL pointer explosion.
>
> I suppose arguably one could argue the builtin memcpy() could deliberately fail
> on an invalid pointer, but that'd be rather ridiculous.
Thank you again, Sean, for committing so much attention to this warning.
Please don't blame it on me.
It would be for the best if I give the full compiler's warning message:
In file included from ./include/linux/string.h:374,
from ./include/linux/bitmap.h:13,
from ./include/linux/cpumask.h:13,
from ./include/linux/alloc_tag.h:13,
from ./include/linux/percpu.h:5,
from ./include/linux/context_tracking_state.h:5,
from ./include/linux/hardirq.h:5,
from ./include/linux/kvm_host.h:7,
from arch/x86/kvm/x86.c:20:
arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’:
./include/linux/fortify-string.h:114:33: error: use of NULL ‘data’ where non-null expected [CWE-476] [-Werror=analyzer-null-argument]
114 | #define __underlying_memcpy __builtin_memcpy
| ^
./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
633 | __underlying_##op(p, q, __fortify_size); \
| ^~~~~~~~~~~~~
./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
| ^~~~~~~~~~~~~~~~~~~~
arch/x86/kvm/x86.c:8773:9: note: in expansion of macro ‘memcpy’
8773 | memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data,
| ^~~~~~
‘kvm_handle_memory_failure’: events 1-4
|
|13649 | int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to ‘kvm_handle_memory_failure’
|......
|13652 | if (r == X86EMUL_PROPAGATE_FAULT) {
| | ~
| | |
| | (2) following ‘false’ branch (when ‘r != 2’)...
|......
|13667 | kvm_prepare_emulation_failure_exit(vcpu);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (3) ...to here
| | (4) calling ‘kvm_prepare_emulation_failure_exit’ from ‘kvm_handle_memory_failure’
|
+--> ‘kvm_prepare_emulation_failure_exit’: events 5-6
|
| 8790 | prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (6) calling ‘prepare_emulation_failure_exit’ from ‘kvm_prepare_emulation_failure_exit’
|......
| 8794 | void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (5) entry to ‘kvm_prepare_emulation_failure_exit’
|
+--> ‘prepare_emulation_failure_exit’: event 7
|
| 8728 | static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, u64 *data,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (7) entry to ‘prepare_emulation_failure_exit’
|
‘prepare_emulation_failure_exit’: event 8
|
|./include/asm-generic/bug.h:112:12:
| 112 | if (unlikely(__ret_warn_on)) \
| | ^
| | |
| | (8) following ‘false’ branch...
arch/x86/kvm/x86.c:8753:13: note: in expansion of macro ‘WARN_ON_ONCE’
| 8753 | if (WARN_ON_ONCE(ndata > 4))
| | ^~~~~~~~~~~~
|
‘prepare_emulation_failure_exit’: events 9-10
|
| 8757 | info_start = 1;
| | ^~~~~~~~~~
| | |
| | (9) ...to here
|......
| 8760 | if (insn_size) {
| | ~
| | |
| | (10) following ‘false’ branch (when ‘insn_size == 0’)...
|
‘prepare_emulation_failure_exit’: event 11
|
|./include/linux/fortify-string.h:620:62:
| 620 | p_size_field, q_size_field, op) ({ \
| | ^
| | |
| | (11) ...to here
./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
| 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
| | ^~~~~~~~~~~~~~~~~~~~
arch/x86/kvm/x86.c:8772:9: note: in expansion of macro ‘memcpy’
| 8772 | memcpy(&run->internal.data[info_start], info, sizeof(info));
| | ^~~~~~
|
‘prepare_emulation_failure_exit’: events 12-17
|
|./include/linux/fortify-string.h:596:12:
| 596 | if (p_size != SIZE_MAX && p_size < size)
| | ^
| | |
| | (12) following ‘false’ branch (when ‘__p_size > 39’)...
| | (14) following ‘false’ branch (when ‘__fortify_size <= __p_size’)...
| 597 | fortify_panic(func, FORTIFY_WRITE, p_size, size, true);
| 598 | else if (q_size != SIZE_MAX && q_size < size)
| | ~~ ~
| | | |
| | | (16) following ‘false’ branch (when ‘__fortify_size <= __q_size’)...
| | (13) ...to here
| | (15) ...to here
|......
| 612 | if (p_size_field != SIZE_MAX &&
| | ~~
| | |
| | (17) ...to here
|
‘prepare_emulation_failure_exit’: event 18
|
| 114 | #define __underlying_memcpy __builtin_memcpy
| | ^
| | |
| | (18) argument 2 (‘data’) NULL where non-null expected
./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
| 633 | __underlying_##op(p, q, __fortify_size); \
| | ^~~~~~~~~~~~~
./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
| 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
| | ^~~~~~~~~~~~~~~~~~~~
arch/x86/kvm/x86.c:8773:9: note: in expansion of macro ‘memcpy’
| 8773 | memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data,
| | ^~~~~~
|
<built-in>: note: argument 2 of ‘__builtin_memcpy’ must be non-null
CC [M] kernel/kheaders.o
cc1: all warnings being treated as errors
Hope this helps.
Best regards,
Mirsad Todorovac
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-07-19 19:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-19 18:22 [BUG] arch/x86/kvm/x86.c: In function ‘prepare_emulation_failure_exit’: error: use of NULL ‘data’ where non-null expected Mirsad Todorovac
2024-07-19 19:01 ` Sean Christopherson
2024-07-19 19:24 ` Mirsad Todorovac
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox