[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

On Thu, Jan 4, 2018 at 9:25 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 04.01.18 at 17:16, <tamas@xxxxxxxxxxxxx> wrote:
>> On Thu, Jan 4, 2018 at 8:00 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 04.01.18 at 15:39, <tamas@xxxxxxxxxxxxx> wrote:
>>>> On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> Just looking at the low bit of the first
>>>>> byte before assuming this could be a load option structure,
>>>>> however, is too weak a check for my taste.
>>>> There is more done in this patch then just checking the low bit of the
>>>> Attributes field: calculating where the OptionalData should be if this
>>>> is indeed an EFI_LOAD_OPTION and then sanity checking if that address
>>>> is sane or not. This requires the buffer to have a layout that at
>>>> least parses properly as an EFI_LOAD_OPTION, hence it likely is one..
>>> But that calculation is not doing all the checking that would be
>>> needed to be reasonably safe. In particular, you mustn't call
>>> wstrlen() when you don't know whether all of the supposed
>>> string is actually inside the given memory block size. And I'm
>>> also not entirely certain whether caring of overflow in only one
>>> (late) place is sufficient.
>> Fair enough. How about if we check whether the supposed string start
>> spot is within the buffer and then use wstrnlen in place such that it
>> can't go past the buffer? Would that be enough sanity checking?
> That should be enough, yes. Do we have wstrnlen() though?
> Rather than introducing it, open-coding the search for the nul
> terminator may be more appropriate in a case like this one.

We don't have it currently. I would prefer introducing it as a
separate function but if you feel strongly about it being implemented
in-place, I can do that.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.