From: Tianyu Lan <[email protected]>
fill_gva_list() populates gva list and adds offset
HV_TLB_FLUSH_UNIT(0x1000000) to variable "cur"
in the each loop. When diff between "end" and "cur" is
less than HV_TLB_FLUSH_UNIT, the gva entry should
be the last one and the loop should be end.
If cur is equal or greater than 0xFF000000 on 32-bit
mode, "cur" will be overflow after adding HV_TLB_FLUSH_UNIT.
Its value will be wrapped and less than "end". fill_gva_list()
falls into an infinite loop and fill gva list out of
border finally.
Set "cur" to be "end" to make loop end when diff is
less than HV_TLB_FLUSH_UNIT and add HV_TLB_FLUSH_UNIT to
"cur" when diff is equal or greater than HV_TLB_FLUSH_UNIT.
Fix the overflow issue.
Reported-by: Jong Hyun Park <[email protected]>
Signed-off-by: Tianyu Lan <[email protected]>
Fixes: 2ffd9e33ce4a ("x86/hyper-v: Use hypercall for remote
TLB flush")
---
arch/x86/hyperv/mmu.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c
index e65d7fe6489f..5208ba49c89a 100644
--- a/arch/x86/hyperv/mmu.c
+++ b/arch/x86/hyperv/mmu.c
@@ -37,12 +37,14 @@ static inline int fill_gva_list(u64 gva_list[], int offset,
* Lower 12 bits encode the number of additional
* pages to flush (in addition to the 'cur' page).
*/
- if (diff >= HV_TLB_FLUSH_UNIT)
+ if (diff >= HV_TLB_FLUSH_UNIT) {
gva_list[gva_n] |= ~PAGE_MASK;
- else if (diff)
+ cur += HV_TLB_FLUSH_UNIT;
+ } else if (diff) {
gva_list[gva_n] |= (diff - 1) >> PAGE_SHIFT;
+ cur = end;
+ }
- cur += HV_TLB_FLUSH_UNIT;
gva_n++;
} while (cur < end);
--
2.14.5
From: [email protected] Sent: Thursday, August 29, 2019 11:16 PM
>
> From: Tianyu Lan <[email protected]>
>
> fill_gva_list() populates gva list and adds offset
> HV_TLB_FLUSH_UNIT(0x1000000) to variable "cur"
> in the each loop. When diff between "end" and "cur" is
> less than HV_TLB_FLUSH_UNIT, the gva entry should
> be the last one and the loop should be end.
>
> If cur is equal or greater than 0xFF000000 on 32-bit
> mode, "cur" will be overflow after adding HV_TLB_FLUSH_UNIT.
> Its value will be wrapped and less than "end". fill_gva_list()
> falls into an infinite loop and fill gva list out of
> border finally.
>
> Set "cur" to be "end" to make loop end when diff is
> less than HV_TLB_FLUSH_UNIT and add HV_TLB_FLUSH_UNIT to
> "cur" when diff is equal or greater than HV_TLB_FLUSH_UNIT.
> Fix the overflow issue.
Let me suggest simplifying the commit message a bit. It
doesn't need to describe every line of the code change. I think
it should also make clear that the same problem could occur on
64-bit systems with the right "start" address. My suggestion:
When the 'start' parameter is >= 0xFF000000 on 32-bit
systems, or >= 0xFFFFFFFF'FF000000 on 64-bit systems,
fill_gva_list gets into an infinite loop. With such inputs,
'cur' overflows after adding HV_TLB_FLUSH_UNIT and always
compares as less than end. Memory is filled with guest virtual
addresses until the system crashes
Fix this by never incrementing 'cur' to be larger than 'end'.
>
> Reported-by: Jong Hyun Park <[email protected]>
> Signed-off-by: Tianyu Lan <[email protected]>
> Fixes: 2ffd9e33ce4a ("x86/hyper-v: Use hypercall for remote
> TLB flush")
The "Fixes:" line needs to not wrap. It's exempt from the
"wrap at 75 columns" rule in order to simplify parsing scripts.
The code itself looks good.
Michael
On Sat, Aug 31, 2019 at 1:41 AM Michael Kelley <[email protected]> wrote:
>
> From: [email protected] Sent: Thursday, August 29, 2019 11:16 PM
> >
> > From: Tianyu Lan <[email protected]>
> >
> > fill_gva_list() populates gva list and adds offset
> > HV_TLB_FLUSH_UNIT(0x1000000) to variable "cur"
> > in the each loop. When diff between "end" and "cur" is
> > less than HV_TLB_FLUSH_UNIT, the gva entry should
> > be the last one and the loop should be end.
> >
> > If cur is equal or greater than 0xFF000000 on 32-bit
> > mode, "cur" will be overflow after adding HV_TLB_FLUSH_UNIT.
> > Its value will be wrapped and less than "end". fill_gva_list()
> > falls into an infinite loop and fill gva list out of
> > border finally.
> >
> > Set "cur" to be "end" to make loop end when diff is
> > less than HV_TLB_FLUSH_UNIT and add HV_TLB_FLUSH_UNIT to
> > "cur" when diff is equal or greater than HV_TLB_FLUSH_UNIT.
> > Fix the overflow issue.
>
> Let me suggest simplifying the commit message a bit. It
> doesn't need to describe every line of the code change. I think
> it should also make clear that the same problem could occur on
> 64-bit systems with the right "start" address. My suggestion:
>
> When the 'start' parameter is >= 0xFF000000 on 32-bit
> systems, or >= 0xFFFFFFFF'FF000000 on 64-bit systems,
> fill_gva_list gets into an infinite loop. With such inputs,
> 'cur' overflows after adding HV_TLB_FLUSH_UNIT and always
> compares as less than end. Memory is filled with guest virtual
> addresses until the system crashes
>
> Fix this by never incrementing 'cur' to be larger than 'end'.
>
> >
> > Reported-by: Jong Hyun Park <[email protected]>
> > Signed-off-by: Tianyu Lan <[email protected]>
> > Fixes: 2ffd9e33ce4a ("x86/hyper-v: Use hypercall for remote
> > TLB flush")
>
> The "Fixes:" line needs to not wrap. It's exempt from the
> "wrap at 75 columns" rule in order to simplify parsing scripts.
>
> The code itself looks good.
Hi Michael:
Thanks for suggestion. Update commit log in V2.
--
Best regards
Tianyu Lan