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

Daniel Vetter
2014-04-25T08:03:04Z

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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch