linux-kernel - Re: [PATCH v4] watchdog: ziirave_wdt: Add support to upload the firmware.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <05209a6b-f709-cf79-2e17-c0dea08f99e6@roeck-us.net>
Date:	Sun, 7 Aug 2016 14:37:00 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Enric Balletbo i Serra <enric.balletbo@...labora.com>,
	linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:	Wim Van Sebroeck <wim@...ana.be>,
	Martyn Welch <martyn.welch@...labora.com>
Subject: Re: [PATCH v4] watchdog: ziirave_wdt: Add support to upload the
 firmware.

On 08/01/2016 04:39 AM, 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>
> ---

[ ... ]

>
> +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_ihex_firmware(&fw, ZIIRAVE_FW_NAME, dev);
> +	if (err) {
> +		dev_err(&client->dev, "Failed to request ihex firmware\n");
> +		return err;
> +	}
> +
> +	err = mutex_lock_interruptible(&w_priv->sysfs_mutex);
> +	if (err)
> +		return err;
> +
This doesn't release the firmware.

> +	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);
> +	mutex_unlock(&w_priv->sysfs_mutex);

Probably best to reverse the order of calls here, and add a second label
to release the firmware without releasing the lock (for use with the above).

Guenter