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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 4, 2018 at 9:45 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 04.01.18 at 17:35, <tamas@xxxxxxxxxxxxx> wrote:
>> 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.
>
> The main reason I wouldn't want it to be introduced in this case
> is that it's not strictly that function that's needed. wmemchr()
> or wstrchr() could both be used as well (and perhaps a few more),
> so which one in particular to use to avoid the open coding I'd like
> to decide at the point when another user of any of the options
> would appear. That way we don't end up with two functions used
> just once. But then again I don't feel _really_ strongly about this.
>

Makes sense.

Tamas

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