| Message ID | 20210108105643.10834-1-shayne.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: fix incorrect strlen of .write in debugfs | expand |
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>
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
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;