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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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