Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

Kumar, Shobhit
2014-04-25T08:24:15Z

On 4/25/2014 1:32 PM, Daniel Vetter wrote:
> On Thu, Apr 24, 2014 at 09:22:23PM +0530, Kumar, Shobhit wrote:
>> On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Be we read and chase pointers from the VBT, it is prudent to make sure
>>> that those accesses are wholly contained within the MMIO region, or else
>>> we may cause a kernel panic during boot.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
>>> 1 file changed, 50 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> index fba9efd..fc9e806 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>>> { }
>>> };
>>>
>>> +static struct bdb_header *validate_vbt(char *base, size_t size,
>>> + struct vbt_header *vbt,
>>> + const char *source)
>>> +{
>>> + size_t offset;
>>> + struct bdb_header *bdb;
>>> +
>>> + if (vbt == NULL) {
>>> + DRM_DEBUG_DRIVER("VBT signature missing\n");
>>> + return NULL;
>>> + }
>>> +
>>> + offset = (char *)vbt - base;
>>> + if (offset + sizeof(struct vbt_header) > size) {
>>> + DRM_DEBUG_DRIVER("VBT header incomplete\n");
>>> + return NULL;
>>> + }
>>> +
>>> + if (memcmp(vbt->signature, "$VBT", 4)) {
>>> + DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>> + return NULL;
>>> + }
>>> +
>>> + offset += vbt->bdb_offset;
>>> + if (offset + sizeof(struct bdb_header) > size) {
>>> + DRM_DEBUG_DRIVER("BDB header incomplete\n");
>>> + return NULL;
>>> + }
>>> +
>>> + bdb = (struct bdb_header *)(base + offset);
>>> + if (offset + bdb->bdb_size > size) {
>>> + DRM_DEBUG_DRIVER("BDB incomplete\n");
>>> + return NULL;
>>> + }
>>
>> I know that BDB version check is really not enough and VBT should be forward
>> compatible, but it would be good to have a version check in driver for the
>> current BDB version the parser supports as well. Strictly speaking if we
>> put this check we should ideally reject any newer versions, but putting an
>> error log indicating mismatch might be a good idea for debug.
>
> Sounds more like an additional patch on top of this one here, so still r-b
> from your side for these changes here?
> -Daniel
>

Hmm, I felt it is something which is missed as part of this patch while
validating VBT. But yes if you feel that it is okay to be a separate
patch then for these changes -

Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

I will then push a new patch on version check.

Regards
Shobhit