This fixes strlen mismatch problems happening in some .write callbacks
of debugfs.
When trying to configure airtime_flags in debugfs, an error appeared:
# echo 0 > /sys/kernel/debug/ieee80211/phy0/airtime_flags
ash: write error: Invalid argument
The error is returned from kstrtou16() since a wrong length makes it
miss the real end of input string. To fix this, use count as the string
length, and set proper end of string for a char buffer.
The debug print is shown - airtime_flags_write: count = 2, len = 8,
where the actual length is 2, but "len = strlen(buf)" gets 8.
Also cleanup the other similar cases for the sake of consistency.
Signed-off-by: Sujuan Chen <[email protected]>
Signed-off-by: Ryder Lee <[email protected]>
Signed-off-by: Shayne Chen <[email protected]>
---
This patch is based on the discussion from
https://patchwork.kernel.org/project/linux-wireless/list/?series=409693
---
net/mac80211/debugfs.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 9135b6f..9991a6a 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -120,7 +120,6 @@ static ssize_t aqm_write(struct file *file,
{
struct ieee80211_local *local = file->private_data;
char buf[100];
- size_t len;
if (count > sizeof(buf))
return -EINVAL;
@@ -128,10 +127,10 @@ static ssize_t aqm_write(struct file *file,
if (copy_from_user(buf, user_buf, count))
return -EFAULT;
- buf[sizeof(buf) - 1] = '\0';
- len = strlen(buf);
- if (len > 0 && buf[len-1] == '\n')
- buf[len-1] = 0;
+ if (count && buf[count - 1] == '\n')
+ buf[count - 1] = '\0';
+
+ buf[count] = '\0';
if (sscanf(buf, "fq_limit %u", &local->fq.limit) == 1)
return count;
@@ -177,7 +176,6 @@ static ssize_t airtime_flags_write(struct file *file,
{
struct ieee80211_local *local = file->private_data;
char buf[16];
- size_t len;
if (count > sizeof(buf))
return -EINVAL;
@@ -185,10 +183,10 @@ static ssize_t airtime_flags_write(struct file *file,
if (copy_from_user(buf, user_buf, count))
return -EFAULT;
- buf[sizeof(buf) - 1] = 0;
- len = strlen(buf);
- if (len > 0 && buf[len - 1] == '\n')
- buf[len - 1] = 0;
+ if (count && buf[count - 1] == '\n')
+ buf[count - 1] = '\0';
+
+ buf[count] = '\0';
if (kstrtou16(buf, 0, &local->airtime_flags))
return -EINVAL;
@@ -237,7 +235,6 @@ static ssize_t aql_txq_limit_write(struct file *file,
{
struct ieee80211_local *local = file->private_data;
char buf[100];
- size_t len;
u32 ac, q_limit_low, q_limit_high, q_limit_low_old, q_limit_high_old;
struct sta_info *sta;
@@ -247,10 +244,10 @@ static ssize_t aql_txq_limit_write(struct file *file,
if (copy_from_user(buf, user_buf, count))
return -EFAULT;
- buf[sizeof(buf) - 1] = 0;
- len = strlen(buf);
- if (len > 0 && buf[len - 1] == '\n')
- buf[len - 1] = 0;
+ if (count && buf[count - 1] == '\n')
+ buf[count - 1] = '\0';
+
+ buf[count] = '\0';
if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) != 3)
return -EINVAL;
@@ -306,7 +303,6 @@ static ssize_t force_tx_status_write(struct file *file,
{
struct ieee80211_local *local = file->private_data;
char buf[3];
- size_t len;
if (count > sizeof(buf))
return -EINVAL;
@@ -314,10 +310,10 @@ static ssize_t force_tx_status_write(struct file *file,
if (copy_from_user(buf, user_buf, count))
return -EFAULT;
- buf[sizeof(buf) - 1] = '\0';
- len = strlen(buf);
- if (len > 0 && buf[len - 1] == '\n')
- buf[len - 1] = 0;
+ if (count && buf[count - 1] == '\n')
+ buf[count - 1] = '\0';
+
+ buf[count] = '\0';
if (buf[0] == '0' && buf[1] == '\0')
local->force_tx_status = 0;
--
2.29.2
Shayne Chen <[email protected]> writes:
> This fixes strlen mismatch problems happening in some .write callbacks
> of debugfs.
>
> When trying to configure airtime_flags in debugfs, an error appeared:
> # echo 0 > /sys/kernel/debug/ieee80211/phy0/airtime_flags
> ash: write error: Invalid argument
>
> The error is returned from kstrtou16() since a wrong length makes it
> miss the real end of input string. To fix this, use count as the string
> length, and set proper end of string for a char buffer.
>
> The debug print is shown - airtime_flags_write: count = 2, len = 8,
> where the actual length is 2, but "len = strlen(buf)" gets 8.
>
> Also cleanup the other similar cases for the sake of consistency.
>
> Signed-off-by: Sujuan Chen <[email protected]>
> Signed-off-by: Ryder Lee <[email protected]>
> Signed-off-by: Shayne Chen <[email protected]>
Acked-by: Toke Høiland-Jørgensen <[email protected]>
This looks wrong to me, am I missing something?
> diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> index 9135b6f..9991a6a 100644
> --- a/net/mac80211/debugfs.c
> +++ b/net/mac80211/debugfs.c
> @@ -120,7 +120,6 @@ static ssize_t aqm_write(struct file *file,
> {
> struct ieee80211_local *local = file->private_data;
> char buf[100];
> - size_t len;
>
> if (count > sizeof(buf))
> return -EINVAL;
This ensures that count <= sizeof(buf)
> @@ -128,10 +127,10 @@ static ssize_t aqm_write(struct file *file,
> if (copy_from_user(buf, user_buf, count))
> return -EFAULT;
We copy, that's fine.
> - buf[sizeof(buf) - 1] = '\0';
> - len = strlen(buf);
> - if (len > 0 && buf[len-1] == '\n')
> - buf[len-1] = 0;
> + if (count && buf[count - 1] == '\n')
> + buf[count - 1] = '\0';
This I think really was meant as strlen, because if you write something
like
10\n\0\0\0\0
before it would have parsed it as 10 still, now it gets confused?
I guess I'm not worried about that though.
> + buf[count] = '\0';
But if count == sizeof(buf) then this is an out-of-bounds write.
Same for all the other copied instances.
johannes
On Fri, 2021-01-08 at 21:02 +0100, Johannes Berg wrote:
> This looks wrong to me, am I missing something?
>
> > diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> > index 9135b6f..9991a6a 100644
> > --- a/net/mac80211/debugfs.c
> > +++ b/net/mac80211/debugfs.c
> > @@ -120,7 +120,6 @@ static ssize_t aqm_write(struct file *file,
> > {
> > struct ieee80211_local *local = file->private_data;
> > char buf[100];
> > - size_t len;
> >
> > if (count > sizeof(buf))
> > return -EINVAL;
>
> This ensures that count <= sizeof(buf)
>
> > @@ -128,10 +127,10 @@ static ssize_t aqm_write(struct file *file,
> > if (copy_from_user(buf, user_buf, count))
> > return -EFAULT;
>
> We copy, that's fine.
>
> > - buf[sizeof(buf) - 1] = '\0';
> > - len = strlen(buf);
> > - if (len > 0 && buf[len-1] == '\n')
> > - buf[len-1] = 0;
> > + if (count && buf[count - 1] == '\n')
> > + buf[count - 1] = '\0';
>
> This I think really was meant as strlen, because if you write something
> like
>
> 10\n\0\0\0\0
>
> before it would have parsed it as 10 still, now it gets confused?
>
> I guess I'm not worried about that though.
I think the problem only happens on airtime_flags_write() that uses
kstrtou16()
> > + buf[count] = '\0';
>
> But if count == sizeof(buf) then this is an out-of-bounds write.
Right. Then, we can
if (count >= sizeof(buf))
return -EINVAL;
Ryder
On Fri, 2021-01-08 at 21:02 +0100, Johannes Berg wrote:
> This looks wrong to me, am I missing something?
>
> > diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> > index 9135b6f..9991a6a 100644
> > --- a/net/mac80211/debugfs.c
> > +++ b/net/mac80211/debugfs.c
> > @@ -120,7 +120,6 @@ static ssize_t aqm_write(struct file *file,
> > {
> > struct ieee80211_local *local = file->private_data;
> > char buf[100];
> > - size_t len;
> >
> > if (count > sizeof(buf))
> > return -EINVAL;
>
> This ensures that count <= sizeof(buf)
>
> > @@ -128,10 +127,10 @@ static ssize_t aqm_write(struct file *file,
> > if (copy_from_user(buf, user_buf, count))
> > return -EFAULT;
>
> We copy, that's fine.
>
> > - buf[sizeof(buf) - 1] = '\0';
> > - len = strlen(buf);
> > - if (len > 0 && buf[len-1] == '\n')
> > - buf[len-1] = 0;
> > + if (count && buf[count - 1] == '\n')
> > + buf[count - 1] = '\0';
>
> This I think really was meant as strlen, because if you write something
> like
>
> 10\n\0\0\0\0
>
> before it would have parsed it as 10 still, now it gets confused?
>
> I guess I'm not worried about that though.
>
Hi Johannes,
Regarding the case "10\n\0\0\0\0", both count and strlen() fail to get
the correct strlen.
# echo "10\n\0\0\0\0" > /sys/kernel/debug/ieee80211/phy0/airtime_flags
airtime_flags_write: count = 13, strlen = 15
> > + buf[count] = '\0';
>
> But if count == sizeof(buf) then this is an out-of-bounds write.
>
> Same for all the other copied instances.
>
> johannes
>
Should we consider this kind of case here?
If yes, maybe we need to use sscanf() as other .write to take care of
this kind of case, since kstrtou16() will also fail on this case.
Btw, some of strlen in other .write are also incorrect, but they won't
get the problem due to sscanf().
Do you prefer that we also use sscanf() in .write of airtime_flags?
Shayne
On Mon, 2021-01-11 at 14:19 +0800, Shayne Chen wrote:
>
> Regarding the case "10\n\0\0\0\0", both count and strlen() fail to get
> the correct strlen.
Yeah.
I don't think we need to worry about this case.
> # echo "10\n\0\0\0\0" > /sys/kernel/debug/ieee80211/phy0/airtime_flags
> airtime_flags_write: count = 13, strlen = 15
> > > + buf[count] = '\0';
> >
> > But if count == sizeof(buf) then this is an out-of-bounds write.
> >
> > Same for all the other copied instances.
> >
> > johannes
> >
>
> Should we consider this kind of case here?
Sure, we're at the kernel/userspace trust boundary, we can't just read
out-of-bounds? Or what do you mean?
johannes
On Mon, 2021-01-11 at 13:10 +0100, Johannes Berg wrote:
> On Mon, 2021-01-11 at 14:19 +0800, Shayne Chen wrote:
> >
> > Regarding the case "10\n\0\0\0\0", both count and strlen() fail to get
> > the correct strlen.
>
> Yeah.
>
> I don't think we need to worry about this case.
>
Got it.
> > # echo "10\n\0\0\0\0" > /sys/kernel/debug/ieee80211/phy0/airtime_flags
> > airtime_flags_write: count = 13, strlen = 15
> > > > + buf[count] = '\0';
> > >
> > > But if count == sizeof(buf) then this is an out-of-bounds write.
> > >
> > > Same for all the other copied instances.
> > >
> > > johannes
> > >
> >
> > Should we consider this kind of case here?
>
> Sure, we're at the kernel/userspace trust boundary, we can't just read
> out-of-bounds? Or what do you mean?
>
> johannes
>
>
Sorry, I put the reply in a wrong place.
I meant should we consider the case "10\n\0\0\0\0" here.
Will send v2, thank you.
Shayne