* [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN
@ 2020-12-22 10:21 Paolo Bonzini
2020-12-22 18:13 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2020-12-22 10:21 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: syzbot+e87846c48bf72bc85311
Since we know that e >= s, we can reassociate the left shift,
changing the shifted number from 1 to 2 in exchange for
decreasing the right hand side by 1.
Reported-by: syzbot+e87846c48bf72bc85311@syzkaller.appspotmail.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/kvm/mmu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 9c4a9c8e43d9..581925e476d6 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -49,7 +49,7 @@ static inline u64 rsvd_bits(int s, int e)
if (e < s)
return 0;
- return ((1ULL << (e - s + 1)) - 1) << s;
+ return ((2ULL << (e - s)) - 1) << s;
}
void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 access_mask);
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN
2020-12-22 10:21 [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN Paolo Bonzini
@ 2020-12-22 18:13 ` Sean Christopherson
2020-12-22 18:31 ` David Laight
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2020-12-22 18:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, syzbot+e87846c48bf72bc85311
On Tue, Dec 22, 2020, Paolo Bonzini wrote:
> Since we know that e >= s, we can reassociate the left shift,
> changing the shifted number from 1 to 2 in exchange for
> decreasing the right hand side by 1.
I assume the edge case is that this ends up as `(1ULL << 64) - 1` and overflows
SHL's max shift count of 63 when s=0 and e=63? If so, that should be called
out. If it's something else entirely, then an explanation is definitely in
order.
> Reported-by: syzbot+e87846c48bf72bc85311@syzkaller.appspotmail.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/mmu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 9c4a9c8e43d9..581925e476d6 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -49,7 +49,7 @@ static inline u64 rsvd_bits(int s, int e)
> if (e < s)
> return 0;
Maybe add a commment? Again assuming my guess about the edge case is on point.
/*
* Use 2ULL to incorporate the necessary +1 in the shift; adding +1 in
* the shift count will overflow SHL's max shift of 63 if s=0 and e=63.
*/
> - return ((1ULL << (e - s + 1)) - 1) << s;
> + return ((2ULL << (e - s)) - 1) << s;
> }
>
> void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 access_mask);
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN
2020-12-22 18:13 ` Sean Christopherson
@ 2020-12-22 18:31 ` David Laight
2020-12-22 22:49 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: David Laight @ 2020-12-22 18:31 UTC (permalink / raw)
To: 'Sean Christopherson', Paolo Bonzini
Cc: linux-kernel, kvm, syzbot+e87846c48bf72bc85311
From: Sean Christopherson
> Sent: 22 December 2020 18:13
>
> On Tue, Dec 22, 2020, Paolo Bonzini wrote:
> > Since we know that e >= s, we can reassociate the left shift,
> > changing the shifted number from 1 to 2 in exchange for
> > decreasing the right hand side by 1.
>
> I assume the edge case is that this ends up as `(1ULL << 64) - 1` and overflows
> SHL's max shift count of 63 when s=0 and e=63? If so, that should be called
> out. If it's something else entirely, then an explanation is definitely in
> order.
>
> > Reported-by: syzbot+e87846c48bf72bc85311@syzkaller.appspotmail.com
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > arch/x86/kvm/mmu.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> > index 9c4a9c8e43d9..581925e476d6 100644
> > --- a/arch/x86/kvm/mmu.h
> > +++ b/arch/x86/kvm/mmu.h
> > @@ -49,7 +49,7 @@ static inline u64 rsvd_bits(int s, int e)
> > if (e < s)
> > return 0;
>
> Maybe add a commment? Again assuming my guess about the edge case is on point.
>
> /*
> * Use 2ULL to incorporate the necessary +1 in the shift; adding +1 in
> * the shift count will overflow SHL's max shift of 63 if s=0 and e=63.
> */
A comment of the desired output value would be more use.
I think it is:
return 'e-s' ones followed by 's' zeros without shifting by 64.
> > - return ((1ULL << (e - s + 1)) - 1) << s;
> > + return ((2ULL << (e - s)) - 1) << s;
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN
2020-12-22 18:31 ` David Laight
@ 2020-12-22 22:49 ` Paolo Bonzini
2020-12-23 16:59 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2020-12-22 22:49 UTC (permalink / raw)
To: David Laight, 'Sean Christopherson'
Cc: linux-kernel, kvm, syzbot+e87846c48bf72bc85311
On 22/12/20 19:31, David Laight wrote:
>> /*
>> * Use 2ULL to incorporate the necessary +1 in the shift; adding +1 in
>> * the shift count will overflow SHL's max shift of 63 if s=0 and e=63.
>> */
> A comment of the desired output value would be more use.
> I think it is:
> return 'e-s' ones followed by 's' zeros without shifting by 64.
>
What about a mix of the two:
/*
* Return 'e-s' ones followed by 's' zeros. Note that the
* apparently obvious 1ULL << (e - s + 1) can shift by 64 if
* s=0 and e=63, which is undefined behavior.
*/
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN
2020-12-22 22:49 ` Paolo Bonzini
@ 2020-12-23 16:59 ` Sean Christopherson
0 siblings, 0 replies; 5+ messages in thread
From: Sean Christopherson @ 2020-12-23 16:59 UTC (permalink / raw)
To: Paolo Bonzini
Cc: David Laight, linux-kernel, kvm, syzbot+e87846c48bf72bc85311
On Tue, Dec 22, 2020, Paolo Bonzini wrote:
> On 22/12/20 19:31, David Laight wrote:
> > > /*
> > > * Use 2ULL to incorporate the necessary +1 in the shift; adding +1 in
> > > * the shift count will overflow SHL's max shift of 63 if s=0 and e=63.
> > > */
> > A comment of the desired output value would be more use.
> > I think it is:
> > return 'e-s' ones followed by 's' zeros without shifting by 64.
> >
>
> What about a mix of the two:
>
> /*
> * Return 'e-s' ones followed by 's' zeros. Note that the
> * apparently obvious 1ULL << (e - s + 1) can shift by 64 if
> * s=0 and e=63, which is undefined behavior.
> */
Works for me, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-23 17:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22 10:21 [PATCH] KVM: x86: fix shift out of bounds reported by UBSAN Paolo Bonzini
2020-12-22 18:13 ` Sean Christopherson
2020-12-22 18:31 ` David Laight
2020-12-22 22:49 ` Paolo Bonzini
2020-12-23 16:59 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).