[Intel-gfx] [PATCH v2 2/6] drm/i915/guc: Add GuC css header parser
Yu Dai
yu.dai at intel.com
Fri Sep 25 09:31:52 PDT 2015
On 09/25/2015 07:45 AM, Jani Nikula wrote:
> On Thu, 24 Sep 2015, Yu Dai <yu.dai at intel.com> wrote:
> > On 09/24/2015 12:04 PM, Dave Gordon wrote:
> >> On 24/09/15 19:34, Yu Dai wrote:
> >> >
> >> >
> >> > On 09/24/2015 07:23 AM, Dave Gordon wrote:
> >> >> On 22/09/15 21:48, yu.dai at intel.com wrote:
> >> >> > From: Alex Dai <yu.dai at intel.com>
> >> >> >
> >> >> > By using information from GuC css header, we can eliminate some
> >> >> > hard code w.r.t size of some components of firmware.
> >> >> >
> >> >> > v2: Add indent into DOC to make fixed-width format rather than
> >> >> > change the tmpl.
> >> >> >
> >> >> > v1: 1) guc_css_header is defined as __packed now
> >> >> > 2) Add and correct GuC related topics in kernel/Doc
> >> >> >
> >> >> > Signed-off-by: Alex Dai <yu.dai at intel.com>
> >> >> > ---
> >> >> > Documentation/DocBook/drm.tmpl | 9 ++-
> >> >> > drivers/gpu/drm/i915/intel_guc.h | 2 +-
> >> >> > drivers/gpu/drm/i915/intel_guc_fwif.h | 36 +++++++++++
> >> >> > drivers/gpu/drm/i915/intel_guc_loader.c | 107
> >> >> ++++++++++++++++++++++----------
> >> >> > 4 files changed, 117 insertions(+), 37 deletions(-)
> >> >> >
> >> >> > diff --git a/Documentation/DocBook/drm.tmpl
> >> >> b/Documentation/DocBook/drm.tmpl
> >> >> > index 66bc646..116332f 100644
> >> >> > --- a/Documentation/DocBook/drm.tmpl
> >> >> > +++ b/Documentation/DocBook/drm.tmpl
> >> >> > @@ -4155,14 +4155,19 @@ int num_ioctls;</synopsis>
> >> >> > <title>GuC-based Command Submission</title>
> >> >> > <sect2>../linux-image-4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly_4.3.0-rc2-dsg-11022-g9765caf-dsg-intel-nightly-7_amd64.deb
> >> >> > <title>GuC</title>
> >> >> > -!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC-specific firmware loader
> >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC
> >> >> > !Idrivers/gpu/drm/i915/intel_guc_loader.c
> >> >> > </sect2>
> >> >> > <sect2>
> >> >> > <title>GuC Client</title>
> >> >> > -!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command
> >> >> submissison
> >> >> > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC Client
> >> >> > !Idrivers/gpu/drm/i915/i915_guc_submission.c
> >> >> > </sect2>
> >> >> > + <sect2>
> >> >> > + <title>GuC Firmware Layout</title>
> >> >> > +!Pdrivers/gpu/drm/i915/intel_guc_loader.c GuC Firmware Layout
> >> >> > +!Idrivers/gpu/drm/i915/intel_guc_loader.c
> >> >> > + </sect2>
> >> >> > </sect1>
> >> >> >
> >> >> > <sect1>
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc.h
> >> >> b/drivers/gpu/drm/i915/intel_guc.h
> >> >> > index 4ec2d27..e1389fc 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_guc.h
> >> >> > +++ b/drivers/gpu/drm/i915/intel_guc.h
> >> >> > @@ -71,6 +71,7 @@ struct intel_guc_fw {
> >> >> > struct drm_i915_gem_object * guc_fw_obj;
> >> >> > enum intel_guc_fw_status guc_fw_fetch_status;
> >> >> > enum intel_guc_fw_status guc_fw_load_status;
> >> >> > + struct guc_css_header guc_fw_header;
> >> >> >
> >> >> > uint16_t guc_fw_major_wanted;
> >> >> > uint16_t guc_fw_minor_wanted;
> >> >> > @@ -80,7 +81,6 @@ struct intel_guc_fw {
> >> >> >
> >> >> > struct intel_guc {
> >> >> > struct intel_guc_fw guc_fw;
> >> >> > -
> >> >> > uint32_t log_flags;
> >> >> > struct drm_i915_gem_object *log_obj;
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
> >> >> b/drivers/gpu/drm/i915/intel_guc_fwif.h
> >> >> > index e1f47ba..006dc0d 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> >> >> > @@ -122,6 +122,42 @@
> >> >> >
> >> >> > #define GUC_CTL_MAX_DWORDS (GUC_CTL_RSRVD + 1)
> >> >> >
> >> >> > +struct guc_css_header {
> >> >> > + uint32_t module_type;
> >> >> > + uint32_t header_len; /* header length plus size of all other
> >> >> keys */
> >> >> > + uint32_t header_version;
> >> >> > + uint32_t module_id;
> >> >> > + uint32_t module_vendor;
> >> >> > + union {
> >> >> > + struct {
> >> >> > + uint8_t day;
> >> >> > + uint8_t month;
> >> >> > + uint16_t year;
> >> >> > + };
> >> >> > + uint32_t date;
> >> >> > + };
> >> >> > + uint32_t size; /* uCode size plus header_len */
> >> >> > + uint32_t key_size;
> >> >> > + uint32_t modulus_size;
> >> >> > + uint32_t exponent_size;
> >> >> > + union {
> >> >> > + struct {
> >> >> > + uint8_t hour;
> >> >> > + uint8_t min;
> >> >> > + uint16_t sec;
> >> >> > + };
> >> >> > + uint32_t time;
> >> >> > + };
> >> >> > +
> >> >> > + char username[8];
> >> >> > + char buildnumber[12];
> >> >> > + uint32_t device_id;
> >> >> > + uint32_t guc_sw_version;
> >> >> > + uint32_t prod_preprod_fw;
> >> >> > + uint32_t reserved[12];
> >> >> > + uint32_t header_info;
> >> >> > +} __packed;
> >> >> > +
> >> >> > struct guc_doorbell_info {
> >> >> > u32 db_status;
> >> >> > u32 cookie;
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >> b/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >> > index 40241f3..a6703b4 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> >> >> > @@ -215,18 +215,29 @@ static inline bool guc_ucode_response(struct
> >> >> drm_i915_private *dev_priv,
> >> >> > ((val & GS_MIA_CORE_STATE) && uk_val ==
> >> >> GS_UKERNEL_LAPIC_DONE));
> >> >> > }
> >> >> >
> >> >> > -/*
> >> >> > - * Transfer the firmware image to RAM for execution by the
> >> >> microcontroller.
> >> >> > +/**
> >> >> > + * DOC: GuC Firmware Layout
> >> >> > *
> >> >> > - * GuC Firmware layout:
> >> >> > - * +-------------------------------+ ----
> >> >> > - * | CSS header | 128B
> >> >> > - * | contains major/minor version |
> >> >> > - * +-------------------------------+ ----
> >> >> > - * | uCode |
> >> >> > - * +-------------------------------+ ----
> >> >> > - * | RSA signature | 256B
> >> >> > - * +-------------------------------+ ----
> >> >> > + * The GuC firmware layout looks like this:
> >> >> > + *
> >> >> > + * +-------------------------------+
> >> >> > + * | guc_css_header |
> >> >> > + * | contains major/minor version |
> >> >> > + * +-------------------------------+
> >> >> > + * | uCode |
> >> >> > + * +-------------------------------+
> >> >> > + * | RSA signature |
> >> >> > + * +-------------------------------+
> >> >> > + * | modulus key |
> >> >> > + * +-------------------------------+
> >> >> > + * | exponent val |
> >> >> > + * +-------------------------------+
> >> >> > + *
> >> >> > + * The firmware may or may not have modulus key and exponent data.
> >> >> The header,
> >> >> > + * uCode and RSA signature are must-have components that will be
> >> >> used by driver.
> >> >> > + * Size of each components (in dwords) can be found in header. In
> >> >> the case that
> >> >> > + * modulus and exponent are not present in fw, the size value still
> >> >> appears in
> >> >> > + * header.
> >> >>
> >> >> I think this picture & commentary belongs just ahead of the structure
> >> >> definition in intel_guc_fwif.h, rather than with the code here. Also,
> >> >
> >> > Yes, this can be moved to intel_guc_fwif.h right before css_header
> >> > structure definition.
> >> >
> >> >> perhaps we nede a bit more explanation of the '*size' fields, since they
> >> >> have been defined in such a counter-intuitive way :( I suggest:
> >> >>
> >> >> * All the 'sizes' in the CSS header are expressed as numbers of
> >> >> * "dwords", and therefore have to be multiplied by sizeof (u32) to
> >> >> * get actual sizes (in the normal sense of byte counts).
> >> >> *
> >> >
> >> > In comments, I clearly mention that all size of these guc components are
> >> > in dwords. In next version, I can add surfix _dword to avoid confuse.
> >>
> >> The names are systematically (if not yet automatically) derived from the
> >> GuC header version, so best not to change them. The comment *in the
> >> header file* will suffice.
> >>
> >> >> * The 'size' field is the total size of the data in the picture
> >> >> * above, and should match the size of the file as provided by the
> >> >> * loader; the file is invalid if this size field is greater than
> >> >> * the actual filesize.
> >> >
> >> > This is not right assumption. And, that will change some expectation
> >> > below related to size checking. I should have make my comments more
> >> > clear. But
> >>
> >> OK, I see, we're going to allow a truncated image file, as long as only
> >> unused sections are missing ...
> >>
> >> > "The firmware may or may not have modulus key and exponent data ... In
> >> > the case that modulus and exponent are not present in fw, the size value
> >> > still appears in header."
> >> >
> >> > We have discussed this with firmware team. Since the 'size' (actually
> >> > the header) is used to generate RSA key, we can't change it after
> >> > signing. So, we have information of modulus etc. but driver doesn't need
> >> > them to load firmware. Likely they are not part of the release bin file.
> >> > So the 'size' may be larger than the bin file size. Currently, fw check
> >> > is based on the following rules (maybe I need add these to the comment
> >> > too):
> >> >
> >> > 1. Header, uCode and RSA are must-have components.
> >> > 2. All firmware components, if they present, are in the sequence
> >> > illustrated in the layout table.
> >> > 3. Size info of each component can be found in header, in dwords.
> >> > 4. Modulus and exponent key are not required by driver. They may not
> >> > appear in fw but their size info are recorded in header anyway.
> >> >
> >> >> * The 'header_len' field contains the total size of the non-uCode
> >> >> * sections of the file (i.e. the sum of the sizes of the CSS header,
> >> >> * the RSA signature, the modulus key and the exponent). Thus, to find
> >> >> * the size of the uCode we subtract this total from 'size', but to
> >> >> * find the size of the CSS header (which also defines the start of
> >> >> * the uCode), we subtract the other three sizes from this total. The
> >> >> * size of the CSS header thus calculated should match sizeof(struct
> >> >> * guc_css_header) (or exceed it; allowing it to be larger permits
> >> >> * future expansion of the CSS header or insertion of extra sections
> >> >> * here). The file is invalid if this calculated size is less than
> >> >> * sizeof(struct guc_css_header).
> >> >> *
> >> >> * The size of the RSA signature must not exceed that expected by the
> >> >> * hardware. The file is invalid if the value of this field is more
> >> >> * than the size of the signature area in the GuC register space,
> >> >> * currently 0x100 bytes.
> >> >> *
> >> >> * Due to the requirements of the DMA engine, the total size of the
> >> >> * sections that are DMA'd into the GuC's memory (CSS header plus
> >> >> * uCode) must be a multiple of the cache line size. The file is
> >> >> * invalid if this requirement is not met.
> >> >
> >> > All suggestions above are valid at some points. However, be note that,
> >> > the firmware size checking I put into code is actually to protect driver
> >> > - just make sure we don't have a bug check due to memory access
> >> > violation. No matter how much protection you put into driver, you still
> >> > can't reject the case by driver if someone modifies one byte of uCode.
> >> > So, as long as we have the bits of header, uCode and RSA, we will load
> >> > it. HW will fail it anyway if anything goes wrong.
> >>
> >> All the checks I suggested were of this type. For example, the change
> >> from using UOS_RSA_SIG_SIZE to the calculated RSA-signature-size would
> >> have allowed a malformed file to trigger writing beyond the 256 bytes of
> >> signature-register space, with unpredictable effects.
> >>
> >> Even if the trailing sections themselves have been removed from the
> >> image file, the assertions about the numerical relationships between the
> >> '*size' values in the CSS header remain valid and should be checked.
> >>
> >> >> The rest of this comment can stay with the code ...
> >> >>
> >> >> > *
> >> >> > * Architecturally, the DMA engine is bidirectional, and can
> >> >> potentially even
> >> >> > * transfer between GTT locations. This functionality is left out
> >> >> of the API
> >> >> > @@ -236,30 +247,42 @@ static inline bool guc_ucode_response(struct
> >> >> drm_i915_private *dev_priv,
> >> >> > * DMA engine in one operation, whereas the RSA signature is
> >> >> loaded via MMIO.
> >> >> > */
> >> >> >
> >> >> > -#define UOS_CSS_HEADER_OFFSET 0
> >> >> > -#define UOS_VER_MINOR_OFFSET 0x44
> >> >> > -#define UOS_VER_MAJOR_OFFSET 0x46
> >> >> > -#define UOS_CSS_HEADER_SIZE 0x80
> >> >> > -#define UOS_RSA_SIG_SIZE 0x100
> >> >> > -
> >> >> > +/*
> >> >> > + * Transfer the firmware image to RAM for execution by the
> >> >> microcontroller.
> >> >> > + */
> >> >> > static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv)
> >> >> > {
> >> >> > struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >> >> > + struct guc_css_header *header = &guc_fw->guc_fw_header;
> >> >> > struct drm_i915_gem_object *fw_obj = guc_fw->guc_fw_obj;
> >> >> > unsigned long offset;
> >> >> > struct sg_table *sg = fw_obj->pages;
> >> >> > - u32 status, ucode_size, rsa[UOS_RSA_SIG_SIZE / sizeof(u32)];
> >> >> > + u32 status, header_size, rsa_size, ucode_size, *rsa;
> >> >> > int i, ret = 0;
> >> >>
> >> >> I don't like doing the complicated size-based calculations in the DMA
> >> >> routine; I'd rather the important values (RSA start/size, CSS+uCode
> >> >> start/size) were precalculated during loading and saved in the struct
> >> >> intel_guc_fw or a member thereof so that this code already has the exact
> >> >> numbers it needs without any further computation.
> >> >
> >> > This is a good point.
> >> >
> >> >> > - /* uCode size, also is where RSA signature starts */
> >> >> > - offset = ucode_size = guc_fw->guc_fw_size - UOS_RSA_SIG_SIZE;
> >> >> > - I915_WRITE(DMA_COPY_SIZE, ucode_size);
> >> >> > + /* The header plus uCode will be copied to WOPCM via DMA,
> >> >> excluding any
> >> >> > + * other components */
> >> >> > + header_size = (header->header_len - header->key_size -
> >> >> > + header->modulus_size - header->exponent_size) * sizeof(u32);
> >> >> > + ucode_size = (header->size - header->header_len) * sizeof(u32);
> >> >> > +
> >> >> > + I915_WRITE(DMA_COPY_SIZE, header_size + ucode_size);
> >> >> > +
> >> >> > + /* where RSA signature starts */
> >> >> > + offset = header_size + ucode_size;
> >> >> > +
> >> >> > + rsa_size = header->key_size * sizeof(u32);
> >> (header->key_size might be, say, 0x80) => rsa_size is 0x200
> >> >> > + rsa = kmalloc(rsa_size, GFP_KERNEL);
> >> >> > + if (!rsa)
> >> >> > + return -ENOMEM;
> >> >> >
> >> >> > /* Copy RSA signature from the fw image to HW for verification */
> >> >> > - sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, UOS_RSA_SIG_SIZE,
> >> >> offset);
> >> >> > - for (i = 0; i < UOS_RSA_SIG_SIZE / sizeof(u32); i++)
> >> >> > + sg_pcopy_to_buffer(sg->sgl, sg->nents, rsa, rsa_size, offset);
> >> >> > + for (i = 0; i < rsa_size / sizeof(u32); i++)
> >> >> > I915_WRITE(UOS_RSA_SCRATCH_0 + i * sizeof(u32), rsa[i]);
> >> i*sizeof(u32) had better not exceed 0x100! OOPS!
> >> >> >
> >> >> > + kfree(rsa);
> >> >> > +
> >> >> > /* Set the source address for the new blob */
> >> >> > offset = i915_gem_obj_ggtt_offset(fw_obj);
> >> >> > I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> >> >> > @@ -465,10 +488,8 @@ static void guc_fw_fetch(struct drm_device
> >> >> *dev, struct intel_guc_fw *guc_fw)
> >> >> > {
> >> >> > struct drm_i915_gem_object *obj;
> >> >> > const struct firmware *fw;
> >> >> > - const u8 *css_header;
> >> >> > - const size_t minsize = UOS_CSS_HEADER_SIZE + UOS_RSA_SIG_SIZE;
> >> >> > - const size_t maxsize = GUC_WOPCM_SIZE_VALUE + UOS_RSA_SIG_SIZE
> >> >> > - - 0x8000; /* 32k reserved (8K stack + 24k context) */
> >> >> > + struct guc_css_header *css_header = &guc_fw->guc_fw_header;
> >> >> > + size_t size;
> >> >> > int err;
> >> >> >
> >> >> > DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch
> >> >> status %s\n",
> >> >> > @@ -482,12 +503,31 @@ static void guc_fw_fetch(struct drm_device
> >> >> *dev, struct intel_guc_fw *guc_fw)
> >> >> >
> >> >> > DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
> >> >> > guc_fw->guc_fw_path, fw);
> >> >> > - DRM_DEBUG_DRIVER("firmware file size %zu (minimum %zu, maximum
> >> >> %zu)\n",
> >> >> > - fw->size, minsize, maxsize);
> >> >> >
> >> >> > - /* Check the size of the blob befoe examining buffer contents */
> >> >> > - if (fw->size < minsize || fw->size > maxsize)
> >> >> > + /* Check the size of the blob before examining buffer contents */
> >> >> > + if (fw->size < sizeof(struct guc_css_header)) {
> >> >> > + DRM_ERROR("Firmware header is missing\n");
> >> >> > + goto fail;
> >> >> > + }
> >> >> > +
> >> >> > + memcpy(css_header, fw->data, sizeof(struct guc_css_header));
> >> >> > +
> >> >> > + /* At least, it should have header, uCode and RSA. Size of all
> >> >> three. */
> >> >> > + size = (css_header->size - css_header->modulus_size -
> >> >> > + css_header->exponent_size) * sizeof(u32);
> >> >> > + if (fw->size < size) {
> >> >> > + DRM_ERROR("Missing firmware components\n");
> >> >> > goto fail;
> >> >> > + }
> >> >> > +
> >> >> > + /* Header and uCode will be loaded to WOPCM. Size of the two. */
> >> >> > + size -= css_header->key_size * sizeof(u32);
> >> >> > +
> >> >> > + /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
> >> >> > + if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) {
> >> >> > + DRM_ERROR("Firmware is too large to fit in WOPCM\n");
> >> >> > + goto fail;
> >> >> > + }
> >> >> >
> >> >> > /*
> >> >> > * The GuC firmware image has the version number embedded at a
> >> >> well-known
> >> >> > @@ -495,9 +535,8 @@ static void guc_fw_fetch(struct drm_device *dev,
> >> >> struct intel_guc_fw *guc_fw)
> >> >> > * TWO bytes each (i.e. u16), although all pointers and
> >> >> offsets are defined
> >> >> > * in terms of bytes (u8).
> >> >> > */
> >> >> > - css_header = fw->data + UOS_CSS_HEADER_OFFSET;
> >> >> > - guc_fw->guc_fw_major_found = *(u16 *)(css_header +
> >> >> UOS_VER_MAJOR_OFFSET);
> >> >> > - guc_fw->guc_fw_minor_found = *(u16 *)(css_header +
> >> >> UOS_VER_MINOR_OFFSET);
> >> >> > + guc_fw->guc_fw_major_found = css_header->guc_sw_version >> 16;
> >> >> > + guc_fw->guc_fw_minor_found = css_header->guc_sw_version & 0xFFFF;
> >> >> >
> >> >> > if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
> >> >> > guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> >> >> >
> >> >>
> >> >> We need to validate each of the sizes read from the binary blob before
> >> >> using them in calculations, and we then also need to validate the
> >> >> results of the calculations, to prevent anyone spoofing the loader into
> >> >> writing where it shouldn't.
> >> >>
> >> >> In particular, we need to check:
> >> >>
> >> >> fw->size >= sizeof(struct guc_css_header)
> >> >> css_header->size (*4) <= fw->size
> >> >> css_header->header_len <= css_header->size
> >> >> => (uCode_size = css_header->size - css_header_len) >= 0
> >> >> css_header->key_size (*4) <= HW_SIG_SIZE (0x100)
> >> >> css_header->key_size <= css_header->header_len
> >> >> css_header->modulus_size <= css_header->header_len
> >> >> css_header->exponent_size <= css_header->header_len
> >> >> css_header->header_len >= css_header->key_size +
> >> >> css_header->modulus_size +
> >> >> css_header->exponent_size;
> >> >> => (css_header_size = css_header->header_len
> >> >> - css_header->key_size
> >> >> - css_header->modulus_size
> >> >> - css_header->exponent_size) >= 0
> >> >> css_header_size + uCode_size == 0 mod cachelinesize
> >> >>
> >> >> (Since all these sizes are unsigned, we can't (and don't need to) check
> >> >> for negative results from the subtractions, but we can check that each
> >> >> value that's defined as including the sum of other values is actually
> >> >> large enough so that the subtractions give meaningful results).
> >> >>
> >> >> Some of these checks are already there, but I think we should complete
> >> >> all of them to catch any other invalid combinations of values. And then
> >> >> save the computed start/size values so the DMA code can just retrieve
> >> >> the precalculated values.
> >> >
> >> > I only agree with check between header size and fw size. This allows
> >> > driver keeps going without bug check. All others between members within
> >> > css_header are not needed. The reason is simple. If you trust content of
> >> > css_header, then you don't need to validate them. If you don't trust it,
> >> > no need to check it anyway. Again, as long as we have enough bits to
> >> > load to HW, we will let it go.
> >> >
> >> > Thanks,
> >> > Alex
> >>
> >> We can't trust the CSS header *unless* we validate it, in the sense of
> >> ensuring that bad values in it won't cause the driver to do anything it
> >> shouldn't (e.g. out of range accesses). For example, what happens if I
> >> set header_len to 0x100 and modulus_size to 0xfffffe00?
> >>
> >
> > We will use whatever provided from header to calculate uCode size and
> > RSA offset etc. If all of them are within the bin file range, we will
> > load it. Otherwise, reject it to avoid SW memory access violation. This
> > is already implemented. My point is: if we can't trust the CSS header,
> > then the check such as (css_header.A >= css_header.B - css_header.C)
> > itself is insignificant. Yes, header data might be corrupted. HW will
> > verify it as part of RSA key authentication anyway.
>
> I guess I was late to the party and commented on an old version [1]. But
> I'm with Dave here; you need to make sure nothing in kernel breaks with
> the data you read from the firmware blob.
>
> It's not immediately obvious that you're safe when you write malicious
> data to DMA_COPY_SIZE, or you write out of bounds of UOS_RSA_SCRATCH_0,
> for example. You *must* make sure they are within sensible limits.
>
Agree. I should have not used the word *trust*. What I mean is presume
the data in header is correct, then calculate size / offset of all
firmware ingredients based on that. If any of them is out of bound,
driver will reject it. In the new version I am working on, the check
related to sizes includes:
1. The fw blob should be larger than sizeof(css_header)
2. The header size should match sizeof(css_header)
3. The RSA length (in dwords) should match UOS_RSA_SCRATCH_MAX_COUNT
(64). This is missing in previous patch. The number 64 (2048 bits)
is from BSpec.
4. The fw blob should have header, ucode and rsa key.
Be note that I will use term *length* or *len* in header definition,
which is in dwords unit. This is to avoid the confusion with *size*.
Thanks,
Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150925/1655347a/attachment-0001.html>
More information about the Intel-gfx
mailing list