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 11:12:06 +0200 Message-ID: <20140425091206.GP26374@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> <20140425080259.GK26374@phenom.ffwll.local> <535A1BA6.7070402@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f41.google.com (mail-ee0-f41.google.com [74.125.83.41]) by gabe.freedesktop.org (Postfix) with ESMTP id ED1146EDF8 for ; Fri, 25 Apr 2014 02:12:11 -0700 (PDT) Received: by mail-ee0-f41.google.com with SMTP id t10so2628746eei.14 for ; Fri, 25 Apr 2014 02:12:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <535A1BA6.7070402@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 Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote: > 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 > >>> > >>>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 > > > > 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 Thanks, patch merged. > I will then push a new patch on version check. I've figured Chris could do it, but if you can take this over I don't think Chris will complain ;-) Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch