diff mbox

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

Message ID 1397855070-4480-7-git-send-email-rodrigo.vivi@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi April 18, 2014, 9:04 p.m. UTC 
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(-)

Comments

Kumar, Shobhit April 24, 2014, 3:52 p.m. UTC | #1 
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.


Regards
Shobhit
Daniel Vetter April 25, 2014, 8:02 a.m. UTC | #2 
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
Kumar, Shobhit April 25, 2014, 8:24 a.m. UTC | #3 
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
Daniel Vetter April 25, 2014, 9:12 a.m. UTC | #4 
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 <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>

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
Chris Wilson April 25, 2014, 9:28 a.m. UTC | #5 
On Fri, Apr 25, 2014 at 11:12:06AM +0200, Daniel Vetter wrote:
> On Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote:
> > On 4/25/2014 1:32 PM, Daniel Vetter wrote:
> > 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 ;-)

I hope that our parser is forward-compatabile and the VBT updates
backwards-compatible... 
-Chris
Kumar, Shobhit April 25, 2014, 11:24 a.m. UTC | #6 
On 4/25/2014 2:58 PM, Chris Wilson wrote:
> On Fri, Apr 25, 2014 at 11:12:06AM +0200, Daniel Vetter wrote:
>> On Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote:
>>> On 4/25/2014 1:32 PM, Daniel Vetter wrote:
>>> 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 ;-)
>
> I hope that our parser is forward-compatabile and the VBT updates
> backwards-compatible...

Yeah of-course, it should be like this.

Regards
Shobhit
diff mbox

Patch

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;
+	}
+
+	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
+		      source, vbt->signature);
+	return bdb;
+}
+
 /**
  * intel_parse_bios - find VBT and initialize settings from the BIOS
  * @dev: DRM device
@@ -1122,20 +1162,13 @@  intel_parse_bios(struct drm_device *dev)
 	init_vbt_defaults(dev_priv);
 
 	/* XXX Should this validation be moved to intel_opregion.c? */
-	if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt) {
-		struct vbt_header *vbt = dev_priv->opregion.vbt;
-		if (memcmp(vbt->signature, "$VBT", 4) == 0) {
-			DRM_DEBUG_KMS("Using VBT from OpRegion: %20s\n",
-					 vbt->signature);
-			bdb = (struct bdb_header *)((char *)vbt + vbt->bdb_offset);
-		} else
-			dev_priv->opregion.vbt = NULL;
-	}
+	if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt)
+		bdb = validate_vbt((char *)dev_priv->opregion.header, OPREGION_SIZE,
+				   (struct vbt_header *)dev_priv->opregion.vbt,
+				   "OpRegion");
 
 	if (bdb == NULL) {
-		struct vbt_header *vbt = NULL;
-		size_t size;
-		int i;
+		size_t i, size;
 
 		bios = pci_map_rom(pdev, &size);
 		if (!bios)
@@ -1143,19 +1176,18 @@  intel_parse_bios(struct drm_device *dev)
 
 		/* Scour memory looking for the VBT signature */
 		for (i = 0; i + 4 < size; i++) {
-			if (!memcmp(bios + i, "$VBT", 4)) {
-				vbt = (struct vbt_header *)(bios + i);
+			if (memcmp(bios + i, "$VBT", 4) == 0) {
+				bdb = validate_vbt(bios, size,
+						   (struct vbt_header *)(bios + i),
+						   "PCI ROM");
 				break;
 			}
 		}
 
-		if (!vbt) {
-			DRM_DEBUG_DRIVER("VBT signature missing\n");
+		if (!bdb) {
 			pci_unmap_rom(pdev, bios);
 			return -1;
 		}
-
-		bdb = (struct bdb_header *)(bios + i + vbt->bdb_offset);
 	}
 
 	/* Grab useful general definitions */