| Message ID | 20170424071157.6979-1-dev@michaelmera.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a16703aaeaedec7a8bee5be5522c7c3e75478951 |
| Delegated to: | Kalle Valo |
| Headers | show |
Michael Mera <dev@michaelmera.com> wrote: > During write to debugfs file simulate_fw_crash, fixed-size local buffer > 'buf' is accessed and modified at index 'count-1', where 'count' is the > size of the write (so potentially out of bounds). > This patch fixes this problem. > > Signed-off-by: Michael Mera <dev@michaelmera.com> I fixed a checkpatch warning in the pending branch: drivers/net/wireless/ath/ath10k/debug.c:636: spaces preferred around that '-' (ctx:VxV)
Kalle Valo <kvalo@qca.qualcomm.com> writes: > I fixed a checkpatch warning in the pending branch: > > drivers/net/wireless/ath/ath10k/debug.c:636: spaces preferred around that '-' (ctx:VxV) Hum... but then you will have a line longer than 80 characters (81), so another warning. I prefered this version since I feel that splitting this line would hurt readability and on the other hand the expression is clearly delimited by the spaces around the ',' so I don't think spaces around the operator add any value (except for compliance with the coding style that is). Thanks, Michael Mera
Michael Mera <dev@michaelmera.com> writes: > Kalle Valo <kvalo@qca.qualcomm.com> writes: >> I fixed a checkpatch warning in the pending branch: >> >> drivers/net/wireless/ath/ath10k/debug.c:636: spaces preferred around >> that '-' (ctx:VxV) > > Hum... but then you will have a line longer than 80 characters (81), so > another warning. I have extended the line length in my check script to 90 chars just because of cases like this: https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle#checking_code > I prefered this version since I feel that splitting this line would hurt > readability and on the other hand the expression is clearly delimited > by the spaces around the ',' so I don't think spaces around the operator > add any value (except for compliance with the coding style that is). I actually guessed why you did that. But I want to keep the output of my test script clean, makes review a lot easier. Making exceptions to the script is more work for me. Anyway, I already did the change in my pending branch and will apply the patch in few days. Thanks for the fix.
Kalle Valo <kvalo@qca.qualcomm.com> writes: > I actually guessed why you did that. But I want to keep the output of my > test script clean, makes review a lot easier. Making exceptions to the > script is more work for me. Yes, sorry. I found the ath10k specific instructions only after submitting the patch. > Anyway, I already did the change in my pending branch and will apply the > patch in few days. Thanks for the fix. Thank you, Michael Mera
Michael Mera <dev@michaelmera.com> wrote: > During write to debugfs file simulate_fw_crash, fixed-size local buffer > 'buf' is accessed and modified at index 'count-1', where 'count' is the > size of the write (so potentially out of bounds). > This patch fixes this problem. > > Signed-off-by: Michael Mera <dev@michaelmera.com> Patch applied to ath-next branch of ath.git, thanks. a16703aaeaed ath10k: fix out of bounds access to local buffer
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c index fb0ade3adb07..7f3c17e55693 100644 --- a/drivers/net/wireless/ath/ath10k/debug.c +++ b/drivers/net/wireless/ath/ath10k/debug.c @@ -628,17 +628,21 @@ static ssize_t ath10k_write_simulate_fw_crash(struct file *file, size_t count, loff_t *ppos) { struct ath10k *ar = file->private_data; - char buf[32]; + char buf[32] = {0}; + ssize_t rc; int ret; - simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count); + /* filter partial writes and invalid commands */ + if (*ppos != 0 || count >= sizeof(buf) || count == 0) + return -EINVAL; - /* make sure that buf is null terminated */ - buf[sizeof(buf) - 1] = 0; + rc = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count); + if (rc < 0) + return rc; /* drop the possible '\n' from the end */ - if (buf[count - 1] == '\n') - buf[count - 1] = 0; + if (buf[*ppos - 1] == '\n') + buf[*ppos - 1] = '\0'; mutex_lock(&ar->conf_mutex);
During write to debugfs file simulate_fw_crash, fixed-size local buffer 'buf' is accessed and modified at index 'count-1', where 'count' is the size of the write (so potentially out of bounds). This patch fixes this problem. Signed-off-by: Michael Mera <dev@michaelmera.com> --- drivers/net/wireless/ath/ath10k/debug.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)