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
