On 03/02/15 08:21, Gao, Liming wrote: > Laszlo: > > So, this is like a clean up. I prefer to defer it until the real > issue or impact is clear. OK for you?
It might be a bit (but not much) more than just cleanup. The consequence seems to be that the allocator wastes some address space (it allocates lower than it would be possible). It's visible if you enable DEBUG_GCD and allocate top-down with an exact match on the high address. But it's definitely not urgent or anything, and the change under discussion might cause regressions, if there's more to the code than immediately apparent. So yes, feel free to postpone this; I just wanted to make you guys aware. Thanks Laszlo > > Thanks > Liming > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Sunday, March 01, 2015 5:10 AM > To: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] possible off-by-one in CoreAllocateSpace()? > > On 02/28/15 10:22, Gao, Liming wrote: >> Laszlo: >> So, your proposal is to change line 1158 if ((Entry->BaseAddress + >> Length) > MaxAddress) { ==> if ((Entry->BaseAddress + Length - 1) > >> MaxAddress) {? > > Yes, it is. > > I didn't write a patch up-front because the code change is easy to do > manually, and (more importantly) I wasn't sure if I was missing something > important. So it was more like a question than a proposal; but yes, the above > is the idea. > > Thanks! > Laszlo > >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, February 26, 2015 2:06 AM >> To: edk2-devel list >> Subject: Re: [edk2] possible off-by-one in CoreAllocateSpace()? >> >> On 02/13/15 13:42, Laszlo Ersek wrote: >>> Hi, >>> >>> there's this code in CoreAllocateSpace(), in >>> "MdeModulePkg/Core/Dxe/Gcd/Gcd.c": >>> >>> 1156 if (GcdAllocateType == >>> EfiGcdAllocateMaxAddressSearchTopDown || >>> 1157 GcdAllocateType == EfiGcdAllocateAnySearchTopDown) { >>> 1158 if ((Entry->BaseAddress + Length) > MaxAddress) { >>> 1159 continue; >>> 1160 } >>> 1161 if (Length > (Entry->EndAddress + 1)) { >>> 1162 Status = EFI_NOT_FOUND; >>> 1163 goto Done; >>> 1164 } >>> 1165 if (Entry->EndAddress > MaxAddress) { >>> 1166 *BaseAddress = MaxAddress; >>> 1167 } else { >>> 1168 *BaseAddress = Entry->EndAddress; >>> 1169 } >>> 1170 *BaseAddress = (*BaseAddress + 1 - Length) & >>> (~AlignmentMask); >>> 1171 } else { >>> 1172 *BaseAddress = (Entry->BaseAddress + AlignmentMask) & >>> (~AlignmentMask); >>> 1173 if ((*BaseAddress + Length - 1) > MaxAddress) { >>> 1174 Status = EFI_NOT_FOUND; >>> 1175 goto Done; >>> 1176 } >>> 1177 } >>> >>> When serching bottom-up (which is the more usual case), the check on line >>> 1173 considers the fact that MaxAddress is an *inclusive* limit, while >>> Length is an *exclusive* size. That is, the following situation is valid: >>> >>> BaseAddress + Length == MaxAddress + 1 >>> >>> which is equivalent to >>> >>> BaseAddress + Length - 1 == MaxAddress >>> >>> Hence the check on line 1173 >>> >>> BaseAddress + Length - 1 > MaxAddress >>> >>> is a correct check for invalidity. Okay. >>> >>> However, the invalidity check on line 1158 seems wrong, for the top-down >>> search: >>> >>> Entry->BaseAddress + Length > MaxAddress >>> >>> is too strict (off-by-one). Namely, >>> >>> BaseAddress + Length == MaxAddress + 1 >>> >>> is a valid condition (see above), but it triggers: >>> >>> BaseAddress + Length == MaxAddress + 1 > MaxAddress >>> >>> I noticed this error by printing GCD maps (DEBUG_GCD) and using >>> EfiGcdAllocateMaxAddressSearchTopDown. >> >> Ping. >> >> This issue is not a security problem, it just pessimizes allocations (it >> wastes address space). >> >> Thanks, >> Laszlo >> >> >> ---------------------------------------------------------------------- >> -------- Dive into the World of Parallel Programming The Go Parallel >> Website, sponsored by Intel and developed in partnership with Slashdot >> Media, is your hub for all things parallel software development, from >> weekly thought leadership blogs to news, videos, case studies, >> tutorials and more. Take a look and join the conversation now. >> http://goparallel.sourceforge.net/ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> ---------------------------------------------------------------------- >> -------- Dive into the World of Parallel Programming The Go Parallel >> Website, sponsored by Intel and developed in partnership with Slashdot >> Media, is your hub for all things parallel software development, from >> weekly thought leadership blogs to news, videos, case studies, >> tutorials and more. Take a look and join the conversation now. >> http://goparallel.sourceforge.net/ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> > > > ------------------------------------------------------------------------------ > Dive into the World of Parallel Programming The Go Parallel Website, > sponsored by Intel and developed in partnership with Slashdot Media, is your > hub for all things parallel software development, from weekly thought > leadership blogs to news, videos, case studies, tutorials and more. Take a > look and join the conversation now. http://goparallel.sourceforge.net/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > Dive into the World of Parallel Programming The Go Parallel Website, sponsored > by Intel and developed in partnership with Slashdot Media, is your hub for all > things parallel software development, from weekly thought leadership blogs to > news, videos, case studies, tutorials and more. Take a look and join the > conversation now. http://goparallel.sourceforge.net/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
