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. 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
