Re: [PATCH] x86/Hyper-V: Fix overflow issue in the fill_gva_list()
On Sat, Aug 31, 2019 at 1:41 AM Michael Kelley wrote: > > From: lantianyu1...@gmail.com Sent: Thursday, August 29, 2019 11:16 PM > > > > From: Tianyu Lan > > > > fill_gva_list() populates gva list and adds offset > > HV_TLB_FLUSH_UNIT(0x100) 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 0xFF00 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 >= 0xFF00 on 32-bit > systems, or >= 0x'FF00 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 > > Signed-off-by: Tianyu Lan > > 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
RE: [PATCH] x86/Hyper-V: Fix overflow issue in the fill_gva_list()
From: lantianyu1...@gmail.com Sent: Thursday, August 29, 2019 11:16 PM > > From: Tianyu Lan > > fill_gva_list() populates gva list and adds offset > HV_TLB_FLUSH_UNIT(0x100) 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 0xFF00 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 >= 0xFF00 on 32-bit systems, or >= 0x'FF00 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 > Signed-off-by: Tianyu Lan > 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
[PATCH] x86/Hyper-V: Fix overflow issue in the fill_gva_list()
From: Tianyu Lan fill_gva_list() populates gva list and adds offset HV_TLB_FLUSH_UNIT(0x100) 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 0xFF00 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 Signed-off-by: Tianyu Lan 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