From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kumar, Shobhit" Subject: Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it Date: Fri, 25 Apr 2014 13:54:06 +0530 Message-ID: <535A1BA6.7070402@intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 2F7D76E15F for ; Fri, 25 Apr 2014 01:24:15 -0700 (PDT) In-Reply-To: <20140425080259.GK26374@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 I will then push a new patch on version check. Regards Shobhit