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


Xen-devel mailing list



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