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@xxxxxxxxx wrote:
>> > From: Alex Dai <yu.dai@xxxxxxxxx>
>> >
>> > 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@xxxxxxxxx>
>> > ---
>> > 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?
