| Message ID | 20210812111056.26926-1-laramglazier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: Fixed size of constant for Windows | expand |
On 8/12/21 1:10 PM, Lara Lazier wrote: > ~0UL has 64 bits on Linux and 32 bits on Windows. > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512". Resolves: https://gitlab.com/qemu-project/qemu/-/issues/512 Fixes: 3407259b20c ("target/i386: Added consistency checks for CR3") > Reported-by: Volker Rümelin <vr_qemu@t-online.de> > Signed-off-by: Lara Lazier <laramglazier@gmail.com> > --- > target/i386/tcg/sysemu/misc_helper.c | 2 +- > target/i386/tcg/sysemu/svm_helper.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c > index 0a7a58ca9a..91b0fc916b 100644 > --- a/target/i386/tcg/sysemu/misc_helper.c > +++ b/target/i386/tcg/sysemu/misc_helper.c > @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) > break; > case 3: > if ((env->efer & MSR_EFER_LMA) && > - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { > + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { Maybe safer to use extract64()? I'm not sure how many bits you want to check, so used TARGET_LONG_BITS: extract64(t0, env_archcpu(env)->phys_bits, TARGET_LONG_BITS - env_archcpu(env)->phys_bits) { > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > } > if (!(env->efer & MSR_EFER_LMA)) { > diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c > index 3891f4e4a8..42cd5053e5 100644 > --- a/target/i386/tcg/sysemu/svm_helper.c > +++ b/target/i386/tcg/sysemu/svm_helper.c > @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) > } > new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); > if ((env->efer & MSR_EFER_LMA) && > - (new_cr3 & ((~0UL) << cpu->phys_bits))) { > + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > } > new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4)); > Any idea how to avoid that class of bug? Some CFLAG or plain checkpatch?
On Thu, 12 Aug 2021 at 12:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 8/12/21 1:10 PM, Lara Lazier wrote: > > ~0UL has 64 bits on Linux and 32 bits on Windows. > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512". > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/512 > Fixes: 3407259b20c ("target/i386: Added consistency checks for CR3") > > > Reported-by: Volker Rümelin <vr_qemu@t-online.de> > > Signed-off-by: Lara Lazier <laramglazier@gmail.com> > > --- > > target/i386/tcg/sysemu/misc_helper.c | 2 +- > > target/i386/tcg/sysemu/svm_helper.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c > > index 0a7a58ca9a..91b0fc916b 100644 > > --- a/target/i386/tcg/sysemu/misc_helper.c > > +++ b/target/i386/tcg/sysemu/misc_helper.c > > @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) > > break; > > case 3: > > if ((env->efer & MSR_EFER_LMA) && > > - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { > > + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { > > Maybe safer to use extract64()? I'm not sure how many bits you want > to check, so used TARGET_LONG_BITS: > > extract64(t0, env_archcpu(env)->phys_bits, > TARGET_LONG_BITS - env_archcpu(env)->phys_bits) { The repetition of the 'phys_bits' expression in both the shift and the length field is pretty awkward. Maybe t0 & ~MAKE_64BIT_MASK(0, env_archcpu(env)->phys_bits) But for this bugfix I think just fixing the suffix is fine. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > > } > > if (!(env->efer & MSR_EFER_LMA)) { > > diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c > > index 3891f4e4a8..42cd5053e5 100644 > > --- a/target/i386/tcg/sysemu/svm_helper.c > > +++ b/target/i386/tcg/sysemu/svm_helper.c > > @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) > > } > > new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); > > if ((env->efer & MSR_EFER_LMA) && > > - (new_cr3 & ((~0UL) << cpu->phys_bits))) { > > + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { > > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > > } > > new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4)); > > > > Any idea how to avoid that class of bug? Some CFLAG or plain checkpatch? In general the 'UL' is often dubious and worth checking -- either 'U' suffices or you wanted 'ULL'. But in a few places where you're actually working with 'unsigned long' it's what you need, so I'm not sure a checkpatch rule would be possible. -- PMM
On 8/12/21 2:54 PM, Peter Maydell wrote: > On Thu, 12 Aug 2021 at 12:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 8/12/21 1:10 PM, Lara Lazier wrote: >>> ~0UL has 64 bits on Linux and 32 bits on Windows. >> >>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512". >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/512 >> Fixes: 3407259b20c ("target/i386: Added consistency checks for CR3") >> >>> Reported-by: Volker Rümelin <vr_qemu@t-online.de> >>> Signed-off-by: Lara Lazier <laramglazier@gmail.com> >>> --- >>> target/i386/tcg/sysemu/misc_helper.c | 2 +- >>> target/i386/tcg/sysemu/svm_helper.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c >>> index 0a7a58ca9a..91b0fc916b 100644 >>> --- a/target/i386/tcg/sysemu/misc_helper.c >>> +++ b/target/i386/tcg/sysemu/misc_helper.c >>> @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) >>> break; >>> case 3: >>> if ((env->efer & MSR_EFER_LMA) && >>> - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { >>> + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { >> >> Maybe safer to use extract64()? I'm not sure how many bits you want >> to check, so used TARGET_LONG_BITS: >> >> extract64(t0, env_archcpu(env)->phys_bits, >> TARGET_LONG_BITS - env_archcpu(env)->phys_bits) { > > The repetition of the 'phys_bits' expression in both the shift and the > length field is pretty awkward. Maybe > > t0 & ~MAKE_64BIT_MASK(0, env_archcpu(env)->phys_bits) Ah, nicer. > But for this bugfix I think just fixing the suffix is fine. Sure. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); >>> } >>> if (!(env->efer & MSR_EFER_LMA)) { >>> diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c >>> index 3891f4e4a8..42cd5053e5 100644 >>> --- a/target/i386/tcg/sysemu/svm_helper.c >>> +++ b/target/i386/tcg/sysemu/svm_helper.c >>> @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) >>> } >>> new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); >>> if ((env->efer & MSR_EFER_LMA) && >>> - (new_cr3 & ((~0UL) << cpu->phys_bits))) { >>> + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { >>> cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); >>> } >>> new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4)); >>> >> >> Any idea how to avoid that class of bug? Some CFLAG or plain checkpatch? > > In general the 'UL' is often dubious and worth checking -- > either 'U' suffices or you wanted 'ULL'. But in a few places > where you're actually working with 'unsigned long' it's what > you need, so I'm not sure a checkpatch rule would be possible. OK, thanks.
On 12/08/21 13:10, Lara Lazier wrote: > ~0UL has 64 bits on Linux and 32 bits on Windows. > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512". > > Reported-by: Volker Rümelin <vr_qemu@t-online.de> > Signed-off-by: Lara Lazier <laramglazier@gmail.com> > --- > target/i386/tcg/sysemu/misc_helper.c | 2 +- > target/i386/tcg/sysemu/svm_helper.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c > index 0a7a58ca9a..91b0fc916b 100644 > --- a/target/i386/tcg/sysemu/misc_helper.c > +++ b/target/i386/tcg/sysemu/misc_helper.c > @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) > break; > case 3: > if ((env->efer & MSR_EFER_LMA) && > - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { > + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > } > if (!(env->efer & MSR_EFER_LMA)) { > diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c > index 3891f4e4a8..42cd5053e5 100644 > --- a/target/i386/tcg/sysemu/svm_helper.c > +++ b/target/i386/tcg/sysemu/svm_helper.c > @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) > } > new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); > if ((env->efer & MSR_EFER_LMA) && > - (new_cr3 & ((~0UL) << cpu->phys_bits))) { > + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > } > new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4)); > Queued, thanks. Paolo
diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c index 0a7a58ca9a..91b0fc916b 100644 --- a/target/i386/tcg/sysemu/misc_helper.c +++ b/target/i386/tcg/sysemu/misc_helper.c @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) break; case 3: if ((env->efer & MSR_EFER_LMA) && - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); } if (!(env->efer & MSR_EFER_LMA)) { diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c index 3891f4e4a8..42cd5053e5 100644 --- a/target/i386/tcg/sysemu/svm_helper.c +++ b/target/i386/tcg/sysemu/svm_helper.c @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) } new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); if ((env->efer & MSR_EFER_LMA) && - (new_cr3 & ((~0UL) << cpu->phys_bits))) { + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); } new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4));
~0UL has 64 bits on Linux and 32 bits on Windows. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512". Reported-by: Volker Rümelin <vr_qemu@t-online.de> Signed-off-by: Lara Lazier <laramglazier@gmail.com> --- target/i386/tcg/sysemu/misc_helper.c | 2 +- target/i386/tcg/sysemu/svm_helper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)