From: Miaoqing Pan <[email protected]>
When the user sets count to zero the string buffer would remain
completely uninitialized which causes the kernel to parse its
own stack data, potentially leading to an info leak. In addition
to that, the string might be not terminated properly when the
user data does not contain a 0-terminator.
Signed-off-by: Miaoqing Pan <[email protected]>
---
drivers/net/wireless/ath/ath9k/tx99.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/ath/ath9k/tx99.c b/drivers/net/wireless/ath/ath9k/tx99.c
index 49ed1af..fe3a826 100644
--- a/drivers/net/wireless/ath/ath9k/tx99.c
+++ b/drivers/net/wireless/ath/ath9k/tx99.c
@@ -179,6 +179,9 @@ static ssize_t write_file_tx99(struct file *file, const char __user *user_buf,
ssize_t len;
int r;
+ if (count < 1)
+ return -EINVAL;
+
if (sc->cur_chan->nvifs > 1)
return -EOPNOTSUPP;
@@ -186,6 +189,8 @@ static ssize_t write_file_tx99(struct file *file, const char __user *user_buf,
if (copy_from_user(buf, user_buf, len))
return -EFAULT;
+ buf[len] = '\0';
+
if (strtobool(buf, &start))
return -EINVAL;
--
1.9.1
>>
>> + buf[len] = '\0';
>> +
>
> I think it would be more appropriate here to check if buf[len] == '\0' and return an error otherwise.
Nevermind, I just had a closer look and I actually think your approach
is fine. I hadn't considered the possibility of someone deliberately
passing a non-null-terminated string with a specific length.
>> Signed-off-by: Miaoqing Pan <[email protected]>
Reviewed-by: Christoph Böhmwalder <[email protected]>
--
Regards,
Christoph
Am 27. September 2017 03:13:34 MESZ schrieb [email protected]:
>From: Miaoqing Pan <[email protected]>
>
>When the user sets count to zero the string buffer would remain
>completely uninitialized which causes the kernel to parse its
>own stack data, potentially leading to an info leak. In addition
>to that, the string might be not terminated properly when the
>user data does not contain a 0-terminator.
>
>Signed-off-by: Miaoqing Pan <[email protected]>
>---
> drivers/net/wireless/ath/ath9k/tx99.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/net/wireless/ath/ath9k/tx99.c
>b/drivers/net/wireless/ath/ath9k/tx99.c
>index 49ed1af..fe3a826 100644
>--- a/drivers/net/wireless/ath/ath9k/tx99.c
>+++ b/drivers/net/wireless/ath/ath9k/tx99.c
>@@ -179,6 +179,9 @@ static ssize_t write_file_tx99(struct file *file,
>const char __user *user_buf,
> ssize_t len;
> int r;
>
>+ if (count < 1)
>+ return -EINVAL;
>+
> if (sc->cur_chan->nvifs > 1)
> return -EOPNOTSUPP;
>
>@@ -186,6 +189,8 @@ static ssize_t write_file_tx99(struct file *file,
>const char __user *user_buf,
> if (copy_from_user(buf, user_buf, len))
> return -EFAULT;
>
>+ buf[len] = '\0';
>+
I think it would be more appropriate here to check if buf[len] == '\0' and return an error otherwise.
> if (strtobool(buf, &start))
> return -EINVAL;
>
>--
>1.9.1
--
Regards,
Christoph
miaoqing pan <[email protected]> wrote:
> When the user sets count to zero the string buffer would remain
> completely uninitialized which causes the kernel to parse its
> own stack data, potentially leading to an info leak. In addition
> to that, the string might be not terminated properly when the
> user data does not contain a 0-terminator.
>
> Signed-off-by: Miaoqing Pan <[email protected]>
> Reviewed-by: Christoph Böhmwalder <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
ee0a47186e2f ath9k: fix tx99 potential info leak
--
https://patchwork.kernel.org/patch/9972889/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches