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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

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.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel