Re: [Intel-gfx] [PATCH v3 02/22] drm/i915/bios: Make copies of VBT data blocks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 07, 2022 at 03:06:04PM +0300, Jani Nikula wrote:
> On Thu, 07 Apr 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Apr 07, 2022 at 01:23:38PM +0300, Jani Nikula wrote:
> >> On Wed, 06 Apr 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > Make a copy of each VB data block with a guaranteed minimum
> >> > size. The extra (if any) will just be left zeroed.
> >> >
> >> > This means we don't have to worry about going out of bounds
> >> > when accessing any of the structure members. Otherwise that
> >> > could easliy happen if we simply get the version check wrong,
> >> > or if the VBT is broken/malicious.
> >> >
> >> > v2: Don't do arithmetic between bdb header and copy
> >> >     of the LFP data block (Jani)
> >> > v3: Make all the copies up front
> >> > v4: Only WARN about min_size==0 if we found the block
> >> >
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_bios.c | 230 ++++++++++++++++------
> >> >  drivers/gpu/drm/i915/i915_drv.h           |   1 +
> >> >  2 files changed, 174 insertions(+), 57 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > index 5518f4cfa1b1..068978734e3b 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> > @@ -88,7 +88,7 @@ static u32 get_blocksize(const void *block_data)
> >> >  }
> >> >  
> >> >  static const void *
> >> > -find_section(const void *_bdb, enum bdb_block_id section_id)
> >> > +find_raw_section(const void *_bdb, enum bdb_block_id section_id)
> >> >  {
> >> >  	const struct bdb_header *bdb = _bdb;
> >> >  	const u8 *base = _bdb;
> >> > @@ -118,6 +118,124 @@ find_section(const void *_bdb, enum bdb_block_id section_id)
> >> >  	return NULL;
> >> >  }
> >> >  
> >> > +/*
> >> > + * Offset from the start of BDB to the start of the
> >> > + * block data (just past the block header).
> >> > + */
> >> > +static u32 block_offset(const void *bdb, enum bdb_block_id section_id)
> >> > +{
> >> > +	const void *block;
> >> > +
> >> > +	block = find_raw_section(bdb, section_id);
> >> > +	if (!block)
> >> > +		return 0;
> >> > +
> >> > +	return block - bdb;
> >> > +}
> >> > +
> >> > +struct bdb_block_entry {
> >> > +	struct list_head node;
> >> > +	enum bdb_block_id section_id;
> >> > +	u8 data[];
> >> > +};
> >> > +
> >> > +static const void *
> >> > +find_section(struct drm_i915_private *i915,
> >> > +	     enum bdb_block_id section_id)
> >> > +{
> >> > +	struct bdb_block_entry *entry;
> >> > +
> >> > +	list_for_each_entry(entry, &i915->vbt.bdb_blocks, node) {
> >> > +		if (entry->section_id == section_id)
> >> > +			return entry->data + 3;
> >> > +	}
> >> > +
> >> 
> >> Failing to find the section_id in the list above, perhaps this should
> >> check if the section_id is present in bdb_blocks[] and complain
> >> loudly. If we fail to add a section there, this will never find
> >> it. I.e. we should never call find_section() on a section_id that isn't
> >> present in bdb_blocks[].
> >
> > That's perfectly legit. Eg. old VBTs are always missing a lot
> > of the eDP related blocks.
> >
> > What we could do is add some kind of dummy block into the list
> > for every block we've initialized, whether we found it or not.
> > With that we could complain if find_section() gets called on
> > anything that we didn't try to duplicate.
> 
> I mean if we don't find it in the list, we can check the bdb_blocks[]
> array if we even have it in our initialization list.

Ah, yeah we could do that I suppose.

-- 
Ville Syrjälä
Intel