[PATCH] mac80211: fix incorrect strlen of .write in debugfs

[PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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(-)

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

[PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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(-)

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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Toke Høiland-Jørgensen]

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>

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Toke Høiland-Jørgensen]

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>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Johannes Berg]

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

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Johannes Berg]

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


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Ryder Lee]

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

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Ryder Lee]

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



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Johannes Berg]

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

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Johannes Berg]

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



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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

Re: [PATCH] mac80211: fix incorrect strlen of .write in debugfs

[Shayne Chen]

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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek