On Sun, 2015-06-28 at 14:49 +0200, Johan Hovold wrote:
> On Sat, Jun 27, 2015 at 03:51:37PM -0500, Peter Berger wrote:
> > On Fri, 2015-06-26 at 09:31 +0200, Johan Hovold wrote:
> > > On Wed, Jun 24, 2015 at 12:18:27AM -0500, Peter Berger wrote:
> > > > On Mon, 2015-06-22 at 11:43 +0200, Johan Hovold wrote:
>
> > > > > Note that sanity checks on the firmware size are missing here; you could
> > > > > fix that as a follow up.
> > > >
> > > > I did some digging and it looks like we can actually do several sanity
> > > > checks on the firmware image. It seems that the Edgeport firmware
> > > > images have a 7-byte header:
> > > > u8 major_version;
> > > > u8 minor_version;
> > > > le16 build_number;
> > > > le16 length;
> > > > u8 checksum;
> > > >
> > > > "length" appears to be the number of bytes of actual data following the
> > > > header.
> > > >
> > > > "checksum" is apparently (from the images I have seen) the low order
> > > > byte resulting from adding the values of all the data bytes.
> > > >
> > > > So, I'm testing a new ck_fw_sanity() function that checks for these
> > > > error conditions:
> > > > 1. NULL (missing) firmware image
> > > > 2. Incomplete firmware header:
> > > > #define FW_HEADER_SIZE 7
> > > > fw->size < FW_HEADER_SIZE
> > > > 3. Bad firmware size:
> > > > fw->size != FW_HEADER_SIZE + length_data
> > > > 4. Bad firmware checksum:
> > > > compute checksum and compare to the stored version
> > > >
> > > > I call ck_fw_sanity() at the top of download_fw(), so, by the time we
> > > > call build_i2c_fw_hdr(), the firmware image has already been sanity
> > > > checked. Initial testing looks good to me. Do you agree that I should
> > > > include my new ck_fw_sanity() function and its invocations (as a new,
> > > > separate 4th patch) in the upcoming v7 patchset?
> > >
> > > That sounds like a good idea, although I suggest you check for NULL
> > > already when requesting the firmware, and then call the verification
> > > function in download_fw.
> >
> > OK. I took the fw == NULL check out of check_fw_sanity() (which is
> > called by download_fw()) and moved it instead to edge_startup() where we
> > call request_firmware().
> >
> > >
> > > Before getting some confirmation from the vendor on steps 3 and 4, you
> > > should probably only warn about these (if anything) but continue the
> > > download anyway.
> >
> > Agreed. I described my guesses about steps 3 and 4 to Digi Tech Support
> > and heard back yesterday saying that they were correct. So in patchset
> > v7 I'm treating these as error conditions. Let me know if you disagree.
>
> That's ok, but make sure to do that as a follow-up patch to the
> firmware-download fix, which we need to backport to stable.
Yes, I've put these changes in a new (compared to the v6 patchset) 4th
patch labeled: "USB: io_ti: Add firmware image sanity checks". I'm
about to send out the new v7 patchset for your consideration.
Thanks.
--Peter
>
> Thanks,
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
