| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFqH_52PikFaWj0fKcqxFu6vTugvwHYEPEKN=YVf198+1rew2g@mail.gmail.com>
Date: Wed, 20 Jul 2016 12:05:58 +0200
From: Enric Balletbo Serra <eballetbo@...il.com>
To: Martyn Welch <martyn.welch@...labora.co.uk>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
Wim Van Sebroeck <wim@...ana.be>,
Guenter Roeck <linux@...ck-us.net>,
Martyn Welch <martyn.welch@...labora.com>
Subject: Re: [PATCH 2/2] watchdog: ziirave_wdt: Add support to upload the firmware.
Hi Martyn,
Many thanks for the review.
2016-07-18 23:43 GMT+02:00 Martyn Welch <martyn.welch@...labora.co.uk>:
> Few comments inline
>
>
> On 17/06/16 11:52, Enric Balletbo i Serra wrote:
>>
>> This patch adds and entry to the sysfs to start firmware upload process
>> on the specified device with the requested firmware.
>>
>> The uploading of the firmware needs only to happen once per firmware
>> upgrade, as the firmware is stored in persistent storage. If the
>> firmware upload or the firmware verification fails then we print and
>> error message and exit.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>> ---
>> drivers/watchdog/ziirave_wdt.c | 426
>> +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 426 insertions(+)
>>
>> diff --git a/drivers/watchdog/ziirave_wdt.c
>> b/drivers/watchdog/ziirave_wdt.c
>> index fa1efef..63e81bc 100644
>> --- a/drivers/watchdog/ziirave_wdt.c
>> +++ b/drivers/watchdog/ziirave_wdt.c
>> @@ -18,7 +18,10 @@
>> * GNU General Public License for more details.
>> */
>>
>> +#include <linux/ctype.h>
>> +#include <linux/delay.h>
>> #include <linux/i2c.h>
>> +#include <linux/firmware.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> @@ -36,6 +39,8 @@
>> #define ZIIRAVE_STATE_OFF 0x1
>> #define ZIIRAVE_STATE_ON 0x2
>>
>> +#define ZIIRAVE_FW_NAME "ziirave_fw.hex"
>> +
>> static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL,
>> NULL,
>> "host request", NULL, "illegal
>> configuration",
>> "illegal instruction", "illegal trap",
>> @@ -50,6 +55,30 @@ static char *ziirave_reasons[] = {"power cycle", "hw
>> watchdog", NULL, NULL,
>> #define ZIIRAVE_WDT_PING 0x9
>> #define ZIIRAVE_WDT_RESET_DURATION 0xa
>>
>> +#define ZIIRAVE_FIRM_PKT_TOTAL_SIZE 20
>> +#define ZIIRAVE_FIRM_PKT_DATA_SIZE 16
>> +#define ZIIRAVE_FIRM_FLASH_MEMORY_START 0x1600
>> +#define ZIIRAVE_FIRM_FLASH_MEMORY_END 0x2bbf
>
>
> This doesn't seem to be used anywhere:
>
Right, this is a possible response but really used here, as i such
case I simply fail. I'll remove it.
>> +/* Download message error. Ex: Message did not CRC properly */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_NAK 0
>> +/* Received and ready for next Download packet. */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_ACK 1
>> +/* Currently writing to flash. Retry Download status in a moment! */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_BUSY 2
>
>
> Neither is this:
>
Yep, same as above.
>> +/* Hardware error writing flash has occurred! */
>> +#define ZIIRAVE_FIRM_DOWNLOAD_FAIL 3
>> +
>> +/*
>> + * Firmware commands
>> + */
>> +#define ZIIRAVE_CMD_DOWNLOAD_START 0x10
>> +#define ZIIRAVE_CMD_DOWNLOAD_END 0x11
>> +#define ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR 0x12
>> +#define ZIIRAVE_CMD_DOWNLOAD_READ_BYTE 0x13
>> +#define ZIIRAVE_CMD_RESET_PROCESSOR 0x0b
>> +#define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER 0x0c
>> +#define ZIIRAVE_CMD_DOWNLOAD_PACKET 0x0e
>> +
>> struct ziirave_wdt_rev {
>> unsigned char major;
>> unsigned char minor;
>> @@ -62,6 +91,23 @@ struct ziirave_wdt_data {
>> int reset_reason;
>> };
>>
>> +/*
>> + * A packet to send to the firmware is composed by following bytes:
>> + * Count | Addr0 | Addr1 | Data0 .. Data15 | Checksum |
>> + * Where,
>
>
> Would "Length" below be the same as "Count" above?
>
Yes.
>
>> + * Length: A data byte containing the length of the data.
>> + * Addr0: Low byte of the address.
>> + * Addr1: High byte of the address.
>> + * Data0 .. Data15: Array of 16 bytes of data.
>> + * Checksum: Checksum byte to verify data integrity.
>> + */
>> +struct ziirave_fw_pkt_t {
>> + u8 length;
>> + u16 addr;
>> + u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
>> + u8 checksum;
>> +} __packed;
>> +
>> static int wdt_timeout;
>> module_param(wdt_timeout, int, 0);
>> MODULE_PARM_DESC(wdt_timeout, "Watchdog timeout in seconds");
>> @@ -146,6 +192,339 @@ static unsigned int ziirave_wdt_get_timeleft(struct
>> watchdog_device *wdd)
>> return ret;
>> }
>>
>> +static int ziirave_firm_wait_for_ack(struct watchdog_device *wdd)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + int ret;
>> +
>> + do {
>> + usleep_range(5000, 5100);
>> + ret = i2c_smbus_read_byte(client);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "Failed to read byte\n");
>> + return ret;
>> + }
>> + } while (ret == ZIIRAVE_FIRM_DOWNLOAD_BUSY);
>> +
>> + return ret == ZIIRAVE_FIRM_DOWNLOAD_ACK ? 0 : -EIO;
>
>
> Is the required response the same for a NAK and FAIL?
>
Well, I'd do the same response, if I don't receive a ACK just fail, I
can add more code here and try to retry if a receive a NAK, but from
experience this adds some error code complexity that I think is not
really useful here. In my test I didn't found a test case that handle
the error in a different way helps.
>
>> +}
>> +
>> +/*
>> + * Parse Microchip format Hex file (an extended Intel Hex file) to
>> extract
>> + * address and data.
>> + */
>> +static int ziirave_firm_parse_hex_line(unsigned char *fw_data,
>> + struct ziirave_fw_pkt_t *fw_pkt,
>> + bool *addr_has_changed)
>> +{
>> + int count = 0;
>> + unsigned char *src, dst;
>> +
>> + if (*fw_data++ != ':')
>> + return -EFAULT;
>> +
>> + /* locate end of line */
>> + for (src = fw_data; *src != '\n'; src += 2) {
>> + /*
>> + * Convert the ascii hexadecimal string to its binary
>> + * representation
>> + */
>> + if (hex2bin(&dst, src, 1))
>> + return -EINVAL;
>> +
>> + /* Parse line to split addr / data */
>> + switch (count) {
>> + case 0:
>> + fw_pkt->length = dst;
>> + break;
>> + case 1:
>> + fw_pkt->addr = dst << 8;
>> + break;
>> + case 2:
>> + fw_pkt->addr |= dst;
>> + fw_pkt->addr >>= 1;
>> + break;
>> + case 3:
>> + /* check if data is an address */
>> + if (dst == 0x04)
>> + *addr_has_changed = true;
>> + else
>> + *addr_has_changed = false;
>> + break;
>> + case 4:
>> + case 5:
>> + if (!*addr_has_changed)
>> + fw_pkt->data[(count - 4)] = dst;
>> + break;
>> + default:
>> + fw_pkt->data[(count - 4)] = dst;
>> + break;
>> + }
>> + count++;
>> + }
>> +
>> + /* return read value + ':' + '\n' */
>> + return (count * 2) + 2;
>> +}
>> +
>> +static int ziirave_firm_set_read_addr(struct watchdog_device *wdd, u16
>> addr)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + u8 address[2];
>> +
>> + memset(address, 0, sizeof(address));
>> + address[0] = addr & 0xff;
>> + address[1] = (addr >> 8) & 0xff;
>> +
>> + return i2c_smbus_write_block_data(client,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_SET_READ_ADDR,
>> + ARRAY_SIZE(address), address);
>> +}
>> +
>> +static int ziirave_firm_write_block_data(struct watchdog_device *wdd,
>> + u8 command, u8 length, const u8
>> *data,
>> + bool wait_for_ack)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + int ret;
>> +
>> + ret = i2c_smbus_write_block_data(client, command, length, data);
>> +
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Failed to send command 0x%02x: %d\n", command,
>> ret);
>> + return ret;
>> + }
>> +
>> + if (wait_for_ack) {
>> + ret = ziirave_firm_wait_for_ack(wdd);
>> + if (ret)
>> + dev_err(&client->dev,
>> + "Failed waiting ACK from command 0x%02x:
>> %d\n",
>> + command, ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int ziirave_firm_write_byte(struct watchdog_device *wdd, u8
>> command,
>> + u8 byte, bool wait_for_ack)
>> +{
>> + return ziirave_firm_write_block_data(wdd, command, 1, &byte,
>> + wait_for_ack);
>> +}
>> +
>> +static int ziirave_firm_write_pkt(struct watchdog_device *wdd,
>> + struct ziirave_fw_pkt_t *fw_pkt)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + u8 i, checksum = 0, packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE];
>> + int ret;
>> +
>> + memset(&packet, 0, sizeof(packet));
>> +
>> + packet[0] = fw_pkt->length;
>> + packet[1] = (u8)(fw_pkt->addr & 0xff);
>> + packet[2] = (u8)((fw_pkt->addr & 0xff00) >> 8);
>> +
>> + /* Copy packet data */
>> + memcpy(packet + 3, fw_pkt->data, fw_pkt->length);
>> +
>> + /* Compute checksum */
>> + for (i = 0; i < (ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1); i++)
>> + checksum += packet[i];
>> + packet[ZIIRAVE_FIRM_PKT_TOTAL_SIZE - 1] = checksum;
>> +
>> + ret = ziirave_firm_write_block_data(wdd,
>> ZIIRAVE_CMD_DOWNLOAD_PACKET,
>> + ARRAY_SIZE(packet), packet,
>> true);
>> + if (ret)
>> + dev_err(&client->dev,
>> + "Failed to write firmware packet at address 0x%04x:
>> %d\n",
>> + fw_pkt->addr, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int ziirave_firm_verify(struct watchdog_device *wdd,
>> + const struct firmware *fw)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + int i, ret, read_bytes = 0, total_read_bytes = 0;
>> + bool addr_has_changed;
>> + struct ziirave_fw_pkt_t fw_pkt;
>> + u8 data[ZIIRAVE_FIRM_PKT_DATA_SIZE];
>> +
>> + while (total_read_bytes < fw->size) {
>> + read_bytes = ziirave_firm_parse_hex_line(
>> + (u8 *)(fw->data +
>> total_read_bytes),
>> + &fw_pkt, &addr_has_changed);
>> +
>> + /* Detect the end of file */
>> + total_read_bytes += read_bytes;
>> + if (total_read_bytes == fw->size)
>> + break;
>> +
>> + if (addr_has_changed)
>> + continue;
>> +
>> + if (fw_pkt.addr < ZIIRAVE_FIRM_FLASH_MEMORY_START ||
>> + fw_pkt.addr > ZIIRAVE_FIRM_FLASH_MEMORY_END)
>> + continue;
>> +
>> + ret = ziirave_firm_set_read_addr(wdd, fw_pkt.addr);
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Failed to send SET_READ_ADDR command:
>> %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(data); i++) {
>> + ret = i2c_smbus_read_byte_data(client,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_READ_BYTE);
>> + if (ret < 0) {
>> + dev_err(&client->dev,
>> + "Failed to READ DATA: %d\n", ret);
>> + return ret;
>> + }
>> + data[i] = (u8)ret;
>> + }
>> +
>> + if (memcmp(data, fw_pkt.data, fw_pkt.length)) {
>> + dev_err(&client->dev,
>> + "Firmware mismatch at address 0x%04x\n",
>> + fw_pkt.addr);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ziirave_firm_upload(struct watchdog_device *wdd,
>> + const struct firmware *fw)
>> +{
>> + struct i2c_client *client = to_i2c_client(wdd->parent);
>> + struct ziirave_fw_pkt_t fw_pkt;
>> + struct ziirave_fw_pkt_t fw_pkt_new;
>> + int ret, total_read_bytes = 0, words_till_page_break;
>> + bool addr_has_changed;
>> +
>> + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER,
>> 1,
>> + false);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(500);
>> +
>> + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1,
>> true);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(500);
>> +
>> + while (total_read_bytes < fw->size) {
>> + int read_bytes = 0;
>> +
>> + read_bytes = ziirave_firm_parse_hex_line(
>> + (u8 *)(fw->data +
>> total_read_bytes),
>> + &fw_pkt, &addr_has_changed);
>> +
>> + if (read_bytes <= 0) {
>> + dev_err(&client->dev,
>> + "Failed to parse HEX file\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Detect the end of file */
>> + total_read_bytes += read_bytes;
>> + if (total_read_bytes == fw->size) {
>> + /*
>> + * For end of download, the length field will be
>> set
>> + * to 0
>> + */
>> + memset(&fw_pkt, 0, sizeof(fw_pkt));
>> + ret = ziirave_firm_write_pkt(wdd, &fw_pkt);
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Failed to send EMPTY packet:
>> %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /* This sleep seems to be required */
>> + msleep(20);
>> +
>> + /* Start firmware verification */
>> + ret = ziirave_firm_verify(wdd, fw);
>> + if (ret) {
>> + dev_err(&client->dev,
>> + "Failed to verify firmware: %d\n",
>> ret);
>> + return ret;
>> + }
>> +
>> + /* End download operation */
>> + ret = ziirave_firm_write_byte(wdd,
>> +
>> ZIIRAVE_CMD_DOWNLOAD_END,
>> + 1, false);
>> + if (ret)
>> + return ret;
>> + break;
>> + }
>> +
>> + if (addr_has_changed)
>> + continue;
>> +
>> + /* Calculate words till page break */
>> + words_till_page_break = (64 - (fw_pkt.addr & 0x3f));
>> + if ((fw_pkt.length >> 1) > words_till_page_break) {
>> + /*
>> + * Data in passes page boundary, so we need to
>> split in
>> + * two blocks of data. Create a packet with the
>> first
>> + * block of data.
>> + */
>> + memset(&fw_pkt_new, 0, sizeof(fw_pkt_new));
>> +
>> + fw_pkt_new.length = words_till_page_break << 1;
>> + fw_pkt_new.addr = fw_pkt.addr;
>> + memcpy(fw_pkt_new.data, fw_pkt.data,
>> fw_pkt_new.length);
>> +
>> + ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new);
>> + if (ret)
>> + return ret;
>> +
>> + /* Create a packet with the second block of data
>> */
>> + memset(&fw_pkt_new, 0, sizeof(fw_pkt_new));
>> +
>> + /* Remaining bytes */
>> + fw_pkt_new.length =
>> + fw_pkt.length - (words_till_page_break <<
>> 1);
>> + fw_pkt_new.addr = fw_pkt.addr +
>> words_till_page_break;
>> + memcpy(fw_pkt_new.data,
>> + fw_pkt.data + (words_till_page_break << 1),
>> + fw_pkt_new.length);
>> +
>> + ret = ziirave_firm_write_pkt(wdd, &fw_pkt_new);
>> + if (ret)
>> + return ret;
>> + } else {
>> + ret = ziirave_firm_write_pkt(wdd, &fw_pkt);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> + /* Reset the processor */
>> + ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_RESET_PROCESSOR, 1,
>> + false);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(500);
>> +
>> + return 0;
>> +}
>> +
>> static const struct watchdog_info ziirave_wdt_info = {
>> .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
>> WDIOF_KEEPALIVEPING,
>> .identity = "Zodiac RAVE Watchdog",
>> @@ -201,10 +580,57 @@ static ssize_t ziirave_wdt_sysfs_show_reason(struct
>> device *dev,
>> static DEVICE_ATTR(reset_reason, S_IRUGO, ziirave_wdt_sysfs_show_reason,
>> NULL);
>>
>> +static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev->parent);
>> + struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
>> + const struct firmware *fw;
>> + int err;
>> +
>> + err = request_firmware(&fw, ZIIRAVE_FW_NAME, dev);
>> + if (err) {
>> + dev_err(&client->dev, "Unable to open firmware %s: %d\n",
>> + ZIIRAVE_FW_NAME, err);
>> + return err;
>> + }
>> +
>> + err = ziirave_firm_upload(&w_priv->wdd, fw);
>> + if (err) {
>> + dev_err(&client->dev, "The firmware update failed: %d\n",
>> err);
>> + goto release_firmware;
>> + }
>> +
>> + /* Update firmware version */
>> + err = ziirave_wdt_revision(client, &w_priv->firmware_rev,
>> + ZIIRAVE_WDT_FIRM_VER_MAJOR);
>> + if (err) {
>> + dev_err(&client->dev, "Failed to read firmware version:
>> %d\n",
>> + err);
>> + goto release_firmware;
>> + }
>> +
>> + dev_info(&client->dev, "Firmware updated to version
>> 02.%02u.%02u\n",
>> + w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
>> +
>> + /* Restore the watchdog timeout */
>> + err = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
>> + if (err)
>> + dev_err(&client->dev, "Failed to set timeout: %d\n", err);
>> +
>> +release_firmware:
>> + release_firmware(fw);
>> + return err ? err : count;
>> +}
>> +
>> +static DEVICE_ATTR(update_fw, S_IWUSR, NULL,
>> ziirave_wdt_sysfs_store_firm);
>> +
>> static struct attribute *ziirave_wdt_attrs[] = {
>> &dev_attr_firmware_version.attr,
>> &dev_attr_bootloader_version.attr,
>> &dev_attr_reset_reason.attr,
>> + &dev_attr_update_fw.attr,
>> NULL
>> };
>> ATTRIBUTE_GROUPS(ziirave_wdt);
>>
>
Powered by blists - more mailing lists