From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it Date: Fri, 25 Apr 2014 10:02:59 +0200 Message-ID: <20140425080259.GK26374@phenom.ffwll.local> References: <1397855070-4480-1-git-send-email-rodrigo.vivi@gmail.com> <1397855070-4480-7-git-send-email-rodrigo.vivi@gmail.com> <53593337.2060709@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f53.google.com (mail-ee0-f53.google.com [74.125.83.53]) by gabe.freedesktop.org (Postfix) with ESMTP id 385356EDDF for ; Fri, 25 Apr 2014 01:03:04 -0700 (PDT) Received: by mail-ee0-f53.google.com with SMTP id b57so2517395eek.26 for ; Fri, 25 Apr 2014 01:03:03 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53593337.2060709@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Kumar, Shobhit" Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > > > >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 > >Signed-off-by: Rodrigo Vivi > >--- > > 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