From: Marek Puzyniak <[email protected]>
Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
In order to have firmware crash simulation functionality also
in firmware 10.1 driver can force firmware crash by performing not allowed
operation. Driver can deliberately crash firmware when setting vdev param for
vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
'soft' which will cause firmware crash that is recoverable
by warm firmware reset but supported only in main firmware.
'hard' which will cause firmware crash recoverable by cold
firmware reset, this option works for both firmwares.
Commands to trigger firmware soft/hard crash:
echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
kvalo: use strncmp(), remove '\n' before checking the command and
document how buf is null terminated
Signed-off-by: Marek Puzyniak <[email protected]>
Signed-off-by: Kalle Valo <[email protected]>
---
drivers/net/wireless/ath/ath10k/debug.c | 51 +++++++++++++++++++++++--------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 6debd281350a..3da70c74b3fa 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -451,12 +451,23 @@ static ssize_t ath10k_read_simulate_fw_crash(struct file *file,
char __user *user_buf,
size_t count, loff_t *ppos)
{
- const char buf[] = "To simulate firmware crash write the keyword"
- " `crash` to this file.\nThis will force firmware"
- " to report a crash to the host system.\n";
+ const char buf[] = "To simulate firmware crash write one of the"
+ " keywords to this file:\n `soft` - this will send"
+ " WMI_FORCE_FW_HANG_ASSERT to firmware if FW"
+ " supports that command.\n `hard` - this will send"
+ " to firmware command with illegal parameters"
+ " causing firmware crash.\n";
+
return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
}
+/* Simulate firmware crash:
+ * 'soft': Call wmi command causing firmware hang. This firmware hang is
+ * recoverable by warm firmware reset.
+ * 'hard': Force firmware crash by setting any vdev parameter for not allowed
+ * vdev id. This is hard firmware crash because it is recoverable only by cold
+ * firmware reset.
+ */
static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
const char __user *user_buf,
size_t count, loff_t *ppos)
@@ -467,11 +478,9 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
mutex_lock(&ar->conf_mutex);
+ /* Don't copy over the last byte, keep it initialised to zero to
+ * make sure that the buffer is properly null terminated. */
simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count);
- if (strcmp(buf, "crash") && strcmp(buf, "crash\n")) {
- ret = -EINVAL;
- goto exit;
- }
if (ar->state != ATH10K_STATE_ON &&
ar->state != ATH10K_STATE_RESTARTED) {
@@ -479,14 +488,30 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
goto exit;
}
- ath10k_info("simulating firmware crash\n");
+ /* drop the possible '\n' from the end */
+ if (buf[count - 1] == '\n') {
+ buf[count - 1] = 0;
+ count--;
+ }
- ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
- if (ret)
- ath10k_warn("failed to force fw hang (%d)\n", ret);
+ if (!strncmp(buf, "soft", sizeof(buf))) {
+ ath10k_info("simulating soft firmware crash\n");
+ ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
+ } else if (!strncmp(buf, "hard", sizeof(buf))) {
+ ath10k_info("simulating hard firmware crash\n");
+ ret = ath10k_wmi_vdev_set_param(ar, TARGET_NUM_VDEVS + 1,
+ ar->wmi.vdev_param->rts_threshold, 0);
+ } else {
+ ret = -EINVAL;
+ goto exit;
+ }
- if (ret == 0)
- ret = count;
+ if (ret) {
+ ath10k_warn("failed to simulate firmware crash: %d\n", ret);
+ goto exit;
+ }
+
+ ret = count;
exit:
mutex_unlock(&ar->conf_mutex);
Dan Carpenter <[email protected]> writes:
>> drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
>> drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
>>
>> I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
>> do here. In my opinion it's guaranteed that the string "hard" is null
>> terminated, so it shouldn't matter even if strlen("soft") (5) is less
>> than sizeof(buf) (32), right? Or am I missing something here?
>>
>
> I am on vacation until next week. I didn't think these particular
> emails went out automatically.
Actually that's true. Fengguang's bot sends me a build report after
every build and I saw these warnings in that report.
> Anyway, I have changed this in the latest smatch but it will take some
> months for the kbuild bot to update.
>
> http://repo.or.cz/w/smatch.git/commitdiff/f10f27a7612c5b1e69b5d9080a0194d012beb6aa
>
> I'll take a closer look next week.
Thanks!
--
Kalle Valo
On Wed, Mar 19, 2014 at 11:06:19AM +0200, Kalle Valo wrote:
> Hi Dan,
>
> Kalle Valo <[email protected]> writes:
>
> > From: Marek Puzyniak <[email protected]>
> >
> > Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
> > In order to have firmware crash simulation functionality also
> > in firmware 10.1 driver can force firmware crash by performing not allowed
> > operation. Driver can deliberately crash firmware when setting vdev param for
> > vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
> >
> > 'soft' which will cause firmware crash that is recoverable
> > by warm firmware reset but supported only in main firmware.
> > 'hard' which will cause firmware crash recoverable by cold
> > firmware reset, this option works for both firmwares.
> >
> > Commands to trigger firmware soft/hard crash:
> >
> > echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
> > echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
> >
> > kvalo: use strncmp(), remove '\n' before checking the command and
> > document how buf is null terminated
> >
> > Signed-off-by: Marek Puzyniak <[email protected]>
> > Signed-off-by: Kalle Valo <[email protected]>
>
> [...]
>
> > @@ -479,14 +488,30 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
> > goto exit;
> > }
> >
> > - ath10k_info("simulating firmware crash\n");
> > + /* drop the possible '\n' from the end */
> > + if (buf[count - 1] == '\n') {
> > + buf[count - 1] = 0;
> > + count--;
> > + }
> >
> > - ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
> > - if (ret)
> > - ath10k_warn("failed to force fw hang (%d)\n", ret);
> > + if (!strncmp(buf, "soft", sizeof(buf))) {
> > + ath10k_info("simulating soft firmware crash\n");
> > + ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
> > + } else if (!strncmp(buf, "hard", sizeof(buf))) {
> > + ath10k_info("simulating hard firmware crash\n");
> > + ret = ath10k_wmi_vdev_set_param(ar, TARGET_NUM_VDEVS + 1,
> > + ar->wmi.vdev_param->rts_threshold, 0);
> > + } else {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
>
> Fengguan's buildbot got warnings here and I assume they are coming from
> smatch:
>
> drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
> drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
>
> I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
> do here. In my opinion it's guaranteed that the string "hard" is null
> terminated, so it shouldn't matter even if strlen("soft") (5) is less
> than sizeof(buf) (32), right? Or am I missing something here?
>
I am on vacation until next week. I didn't think these particular
emails went out automatically. Anyway, I have changed this in the
latest smatch but it will take some months for the kbuild bot to update.
http://repo.or.cz/w/smatch.git/commitdiff/f10f27a7612c5b1e69b5d9080a0194d012beb6aa
I'll take a closer look next week.
regards,
dan carpenter
On Tue, Mar 25, 2014 at 08:52:14AM +0200, Kalle Valo wrote:
> Dan Carpenter <[email protected]> writes:
>
> >> drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
> >> drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
> >>
> >> I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
> >> do here. In my opinion it's guaranteed that the string "hard" is null
> >> terminated, so it shouldn't matter even if strlen("soft") (5) is less
> >> than sizeof(buf) (32), right? Or am I missing something here?
> >>
> >
> > I am on vacation until next week. I didn't think these particular
> > emails went out automatically.
>
> Actually that's true. Fengguang's bot sends me a build report after
> every build and I saw these warnings in that report.
>
> > Anyway, I have changed this in the latest smatch but it will take some
> > months for the kbuild bot to update.
> >
> > http://repo.or.cz/w/smatch.git/commitdiff/f10f27a7612c5b1e69b5d9080a0194d012beb6aa
> >
> > I'll take a closer look next week.
>
> Thanks!
Actually I was just looking for why the email went out but you already
answered the question. Eventually, this warning will go away when the
build-bot updates. (It's sort of painful to update because Smatch
doesn't have stable releases).
regards,
dan carpenter
On Wed, Mar 19, 2014 at 11:16 AM, Michal Kazior <[email protected]> wrote:
>
> On 19 March 2014 10:06, Kalle Valo <[email protected]> wrote:
> > Fengguan's buildbot got warnings here and I assume they are coming from
> > smatch:
> >
> > drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
> > drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
> >
> > I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
> > do here. In my opinion it's guaranteed that the string "hard" is null
> > terminated, so it shouldn't matter even if strlen("soft") (5) is less
> > than sizeof(buf) (32), right? Or am I missing something here?
>
> Hmm.. strncmp() compares *at most* n chars. The above means you can
> overflow the const char[] "hard" and "soft" if `buf` is longer than
> those. strncmp() must be passed the smallest length of either
> argument.
No you cannot. strncmp() will stop at '\0' from either buffer. If buf
is nul terminated, I see no point in using strncmp here (i.e., strcmp
should be used instead). If buf is not nul terminated, the length to
strncmp() must be the correct length of data in that buf, not
sizeof(buf). In either case, the current version here is incorrect
(even if it could not result in a buffer read overflow in practice).
- Jouni
Jouni Malinen <[email protected]> writes:
> On Wed, Mar 19, 2014 at 11:16 AM, Michal Kazior <[email protected]> wrote:
>>
>> On 19 March 2014 10:06, Kalle Valo <[email protected]> wrote:
>> > Fengguan's buildbot got warnings here and I assume they are coming from
>> > smatch:
>> >
>> > drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
>> > drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
>> >
>> > I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
>> > do here. In my opinion it's guaranteed that the string "hard" is null
>> > terminated, so it shouldn't matter even if strlen("soft") (5) is less
>> > than sizeof(buf) (32), right? Or am I missing something here?
>>
>> Hmm.. strncmp() compares *at most* n chars. The above means you can
>> overflow the const char[] "hard" and "soft" if `buf` is longer than
>> those. strncmp() must be passed the smallest length of either
>> argument.
>
> No you cannot. strncmp() will stop at '\0' from either buffer. If buf
> is nul terminated, I see no point in using strncmp here (i.e., strcmp
> should be used instead). If buf is not nul terminated, the length to
> strncmp() must be the correct length of data in that buf, not
> sizeof(buf). In either case, the current version here is incorrect
> (even if it could not result in a buffer read overflow in practice).
The reason why I changed strcmp() to strncmp() was to avoid the case if
someone accidentally removes "- 1" from the copy. I think this is just
too subtle for mistakes:
char buf[32] = {};
[...]
/* Don't copy over the last byte, keep it initialised to zero to
* make sure that the buffer is properly null terminated. */
simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count);
So I considered strncmp() just as an extra layer of protection (it has a
limit for the loop). But now that I think more about this, maybe this is
safer:
char buf[10];
[...]
simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
/* make sure buf is null terminated */
buf[sizeof(buf) - 1] = 0;
But anyway, I'll change the patch to use strcmp() again.
--
Kalle Valo
Hi Kalle,
On 14 March 2014 11:39, Kalle Valo <[email protected]> wrote:
> Hi Marek,
>
> Kalle Valo <[email protected]> writes:
>
>> From: Marek Puzyniak <[email protected]>
>>
>> Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
>> In order to have firmware crash simulation functionality also
>> in firmware 10.1 driver can force firmware crash by performing not allowed
>> operation. Driver can deliberately crash firmware when setting vdev param for
>> vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
[...]
> can you please review my changes and check if they are ok?
Looks fine.
Thank you,
Marek
Kalle Valo <[email protected]> writes:
> From: Marek Puzyniak <[email protected]>
>
> Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
> In order to have firmware crash simulation functionality also
> in firmware 10.1 driver can force firmware crash by performing not allowed
> operation. Driver can deliberately crash firmware when setting vdev param for
> vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
>
> 'soft' which will cause firmware crash that is recoverable
> by warm firmware reset but supported only in main firmware.
> 'hard' which will cause firmware crash recoverable by cold
> firmware reset, this option works for both firmwares.
>
> Commands to trigger firmware soft/hard crash:
>
> echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
> echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
>
> kvalo: use strncmp(), remove '\n' before checking the command and
> document how buf is null terminated
>
> Signed-off-by: Marek Puzyniak <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Thanks, applied. I just removed by strncmp() changes based on the
discussion.
--
Kalle Valo
Hi Marek,
Kalle Valo <[email protected]> writes:
> From: Marek Puzyniak <[email protected]>
>
> Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
> In order to have firmware crash simulation functionality also
> in firmware 10.1 driver can force firmware crash by performing not allowed
> operation. Driver can deliberately crash firmware when setting vdev param for
> vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
>
> 'soft' which will cause firmware crash that is recoverable
> by warm firmware reset but supported only in main firmware.
> 'hard' which will cause firmware crash recoverable by cold
> firmware reset, this option works for both firmwares.
>
> Commands to trigger firmware soft/hard crash:
>
> echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
> echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
>
> kvalo: use strncmp(), remove '\n' before checking the command and
> document how buf is null terminated
>
> Signed-off-by: Marek Puzyniak <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
can you please review my changes and check if they are ok?
--
Kalle Valo
Hi Dan,
Kalle Valo <[email protected]> writes:
> From: Marek Puzyniak <[email protected]>
>
> Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
> In order to have firmware crash simulation functionality also
> in firmware 10.1 driver can force firmware crash by performing not allowed
> operation. Driver can deliberately crash firmware when setting vdev param for
> vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
>
> 'soft' which will cause firmware crash that is recoverable
> by warm firmware reset but supported only in main firmware.
> 'hard' which will cause firmware crash recoverable by cold
> firmware reset, this option works for both firmwares.
>
> Commands to trigger firmware soft/hard crash:
>
> echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
> echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
>
> kvalo: use strncmp(), remove '\n' before checking the command and
> document how buf is null terminated
>
> Signed-off-by: Marek Puzyniak <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
[...]
> @@ -479,14 +488,30 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
> goto exit;
> }
>
> - ath10k_info("simulating firmware crash\n");
> + /* drop the possible '\n' from the end */
> + if (buf[count - 1] == '\n') {
> + buf[count - 1] = 0;
> + count--;
> + }
>
> - ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
> - if (ret)
> - ath10k_warn("failed to force fw hang (%d)\n", ret);
> + if (!strncmp(buf, "soft", sizeof(buf))) {
> + ath10k_info("simulating soft firmware crash\n");
> + ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
> + } else if (!strncmp(buf, "hard", sizeof(buf))) {
> + ath10k_info("simulating hard firmware crash\n");
> + ret = ath10k_wmi_vdev_set_param(ar, TARGET_NUM_VDEVS + 1,
> + ar->wmi.vdev_param->rts_threshold, 0);
> + } else {
> + ret = -EINVAL;
> + goto exit;
> + }
Fengguan's buildbot got warnings here and I assume they are coming from
smatch:
drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
do here. In my opinion it's guaranteed that the string "hard" is null
terminated, so it shouldn't matter even if strlen("soft") (5) is less
than sizeof(buf) (32), right? Or am I missing something here?
--
Kalle Valo
On 19 March 2014 10:06, Kalle Valo <[email protected]> wrote:
> Hi Dan,
>
> Kalle Valo <[email protected]> writes:
>
>> From: Marek Puzyniak <[email protected]>
>>
>> Command WMI_FORCE_FW_HANG_CMDID is not supported in firmware 10.1.
>> In order to have firmware crash simulation functionality also
>> in firmware 10.1 driver can force firmware crash by performing not allowed
>> operation. Driver can deliberately crash firmware when setting vdev param for
>> vdev id out of range. This patch introduces two keywords to simulate_fw_crash:
>>
>> 'soft' which will cause firmware crash that is recoverable
>> by warm firmware reset but supported only in main firmware.
>> 'hard' which will cause firmware crash recoverable by cold
>> firmware reset, this option works for both firmwares.
>>
>> Commands to trigger firmware soft/hard crash:
>>
>> echo 'soft' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
>> echo 'hard' > /sys/kernel/debug/ieee80211/phyX/ath10k/simulate_fw_crash
>>
>> kvalo: use strncmp(), remove '\n' before checking the command and
>> document how buf is null terminated
>>
>> Signed-off-by: Marek Puzyniak <[email protected]>
>> Signed-off-by: Kalle Valo <[email protected]>
>
> [...]
>
>> @@ -479,14 +488,30 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
>> goto exit;
>> }
>>
>> - ath10k_info("simulating firmware crash\n");
>> + /* drop the possible '\n' from the end */
>> + if (buf[count - 1] == '\n') {
>> + buf[count - 1] = 0;
>> + count--;
>> + }
>>
>> - ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
>> - if (ret)
>> - ath10k_warn("failed to force fw hang (%d)\n", ret);
>> + if (!strncmp(buf, "soft", sizeof(buf))) {
>> + ath10k_info("simulating soft firmware crash\n");
>> + ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
>> + } else if (!strncmp(buf, "hard", sizeof(buf))) {
>> + ath10k_info("simulating hard firmware crash\n");
>> + ret = ath10k_wmi_vdev_set_param(ar, TARGET_NUM_VDEVS + 1,
>> + ar->wmi.vdev_param->rts_threshold, 0);
>> + } else {
>> + ret = -EINVAL;
>> + goto exit;
>> + }
>
> Fengguan's buildbot got warnings here and I assume they are coming from
> smatch:
>
> drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
> drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
>
> I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
> do here. In my opinion it's guaranteed that the string "hard" is null
> terminated, so it shouldn't matter even if strlen("soft") (5) is less
> than sizeof(buf) (32), right? Or am I missing something here?
Hmm.. strncmp() compares *at most* n chars. The above means you can
overflow the const char[] "hard" and "soft" if `buf` is longer than
those. strncmp() must be passed the smallest length of either
argument.
Michał