[PATCH] mac80211: Fix the buffer length in debugfs for smps

[PATCH] mac80211: Fix the buffer length in debugfs for smps

[Chaitanya T K]

This was blocking sending SMPS action frames 
through debugfs.

Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
---
 net/mac80211/debugfs_netdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index 04b5a14..1a8fa5f 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -265,7 +265,7 @@ static ssize_t ieee80211_if_parse_smps(struct ieee80211_sub_if_data *sdata,
 	enum ieee80211_smps_mode mode;
 
 	for (mode = 0; mode < IEEE80211_SMPS_NUM_MODES; mode++) {
-		if (strncmp(buf, smps_modes[mode], buflen) == 0) {
+		if (strncmp(buf, smps_modes[mode], buflen-1) == 0) {
 			int err = ieee80211_set_smps(sdata, mode);
 			if (!err)
 				return buflen;

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Johannes Berg]

On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
> This was blocking sending SMPS action frames 
> through debugfs.

I don't see any issue here, explain.

johannes

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Krishna Chaitanya]

On Mon, Jan 6, 2014 at 8:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
>> This was blocking sending SMPS action frames
>> through debugfs.
>
> I don't see any issue here, explain.
>
> johannes
>
buflen includes the new line character as well, hence the comparison
strncmp fails for all combiantions.

echo "static" > ieee80211/phyX/netdev\:wlanX/smps
Then

buf=static\n
buflen=7

But the comparison is with "static" which doesn't include "\n"
hence the comparison fails.

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Johannes Berg]

On Mon, 2014-01-06 at 20:35 +0530, Krishna Chaitanya wrote:
> On Mon, Jan 6, 2014 at 8:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
> >> This was blocking sending SMPS action frames
> >> through debugfs.
> >
> > I don't see any issue here, explain.
> >
> > johannes
> >
> buflen includes the new line character as well, hence the comparison
> strncmp fails for all combiantions.
> 
> echo "static" > ieee80211/phyX/netdev\:wlanX/smps
> Then
> 
> buf=static\n
> buflen=7
> 
> But the comparison is with "static" which doesn't include "\n"
> hence the comparison fails.

Err, ok, so you can just do "echo -n static", right?

And then your patch breaks the way it currently works, which is about
the worst you can do.

johannes

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Krishna Chaitanya]

On Mon, Jan 6, 2014 at 8:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-01-06 at 20:35 +0530, Krishna Chaitanya wrote:
>> On Mon, Jan 6, 2014 at 8:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
>> >> This was blocking sending SMPS action frames
>> >> through debugfs.
>> >
>> > I don't see any issue here, explain.
>> >
>> > johannes
>> >
>> buflen includes the new line character as well, hence the comparison
>> strncmp fails for all combiantions.
>>
>> echo "static" > ieee80211/phyX/netdev\:wlanX/smps
>> Then
>>
>> buf=static\n
>> buflen=7
>>
>> But the comparison is with "static" which doesn't include "\n"
>> hence the comparison fails.
>
> Err, ok, so you can just do "echo -n static", right?
>
> And then your patch breaks the way it currently works, which is about
> the worst you can do.
>
Ok, if one works other fails.

So instead why cant we use strlen(local_string)
instead of using buflen(remote). That way we can make sure we only
compare the characters we need and leave out the extra ones.

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Johannes Berg]

On Mon, 2014-01-06 at 21:02 +0530, Krishna Chaitanya wrote:
> On Mon, Jan 6, 2014 at 8:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2014-01-06 at 20:35 +0530, Krishna Chaitanya wrote:
> >> On Mon, Jan 6, 2014 at 8:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> >> > On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
> >> >> This was blocking sending SMPS action frames
> >> >> through debugfs.
> >> >
> >> > I don't see any issue here, explain.
> >> >
> >> > johannes
> >> >
> >> buflen includes the new line character as well, hence the comparison
> >> strncmp fails for all combiantions.
> >>
> >> echo "static" > ieee80211/phyX/netdev\:wlanX/smps
> >> Then
> >>
> >> buf=static\n
> >> buflen=7
> >>
> >> But the comparison is with "static" which doesn't include "\n"
> >> hence the comparison fails.
> >
> > Err, ok, so you can just do "echo -n static", right?
> >
> > And then your patch breaks the way it currently works, which is about
> > the worst you can do.
> >
> Ok, if one works other fails.
> 
> So instead why cant we use strlen(local_string)
> instead of using buflen(remote). That way we can make sure we only
> compare the characters we need and leave out the extra ones.

It wouldn't fix the problem and would introduce a buffer overflow bug.

johannes

Re: [PATCH] mac80211: Fix the buffer length in debugfs for smps

[Krishna Chaitanya]

On Mon, Jan 6, 2014 at 9:25 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-01-06 at 21:02 +0530, Krishna Chaitanya wrote:
>> On Mon, Jan 6, 2014 at 8:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Mon, 2014-01-06 at 20:35 +0530, Krishna Chaitanya wrote:
>> >> On Mon, Jan 6, 2014 at 8:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> >> > On Sun, 2014-01-05 at 02:36 +0530, Chaitanya T K wrote:
>> >> >> This was blocking sending SMPS action frames
>> >> >> through debugfs.
>> >> >
>> >> > I don't see any issue here, explain.
>> >> >
>> >> > johannes
>> >> >
>> >> buflen includes the new line character as well, hence the comparison
>> >> strncmp fails for all combiantions.
>> >>
>> >> echo "static" > ieee80211/phyX/netdev\:wlanX/smps
>> >> Then
>> >>
>> >> buf=static\n
>> >> buflen=7
>> >>
>> >> But the comparison is with "static" which doesn't include "\n"
>> >> hence the comparison fails.
>> >
>> > Err, ok, so you can just do "echo -n static", right?
>> >
>> > And then your patch breaks the way it currently works, which is about
>> > the worst you can do.
>> >
>> Ok, if one works other fails.
>>
>> So instead why cant we use strlen(local_string)
>> instead of using buflen(remote). That way we can make sure we only
>> compare the characters we need and leave out the extra ones.
>
> It wouldn't fix the problem and would introduce a buffer overflow bug.
>
We can add some checks to make sure it doesn't overflow, but its not
worth it.

My intention is as most of the users are familiar with "echo" and
not "echo -n", its better to have a solution which works with "echo".

Also if we use buflen-1 and give "echo -n" it still works but problem is
it compares 1 less character.

Anyways either one is fine with me.

Thanks.