diff mbox series

mac80211: fix incorrect strlen of .write in debugfs

Message ID 20210108105643.10834-1-shayne.chen@mediatek.com
State New
Headers show
Series mac80211: fix incorrect strlen of .write in debugfs | expand

Commit Message

Shayne Chen Jan. 8, 2021, 10:56 a.m. UTC
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 <sujuan.chen@mediatek.com>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Shayne Chen <shayne.chen@mediatek.com>
---
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(-)

Comments

Toke Høiland-Jørgensen Jan. 8, 2021, 12:27 p.m. UTC | #1
Shayne Chen <shayne.chen@mediatek.com> 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 <sujuan.chen@mediatek.com>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> Signed-off-by: Shayne Chen <shayne.chen@mediatek.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Ryder Lee Jan. 9, 2021, 12:22 a.m. UTC | #2
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
Shayne Chen Jan. 11, 2021, 6:19 a.m. UTC | #3
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
Johannes Berg Jan. 11, 2021, 12:10 p.m. UTC | #4
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
Shayne Chen Jan. 12, 2021, 1:42 a.m. UTC | #5
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
diff mbox series

Patch

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;