* [PATCH] rt2x00: unterminated strlen of user data @ 2016-02-15 19:04 Alan 2016-02-16 6:36 ` Souptick Joarder 2016-02-17 0:09 ` Julian Calaby 0 siblings, 2 replies; 7+ messages in thread From: Alan @ 2016-02-15 19:04 UTC (permalink / raw) To: linux-wireless, kvalo The buffer needs to be zero terminated in case the user data is not. Otherwise we run off the end of the buffer. Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c index 25ee3cb..72ae530 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ { \ struct rt2x00debug_intf *intf = file->private_data; \ const struct rt2x00debug *debug = intf->debug; \ - char line[16]; \ + char line[17]; \ size_t size; \ unsigned int index = intf->offset_##__name; \ __type value; \ @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ \ if (copy_from_user(line, buf, length)) \ return -EFAULT; \ - \ + line[16] = 0; \ + \ size = strlen(line); \ value = simple_strtoul(line, NULL, 0); \ \ ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-15 19:04 [PATCH] rt2x00: unterminated strlen of user data Alan @ 2016-02-16 6:36 ` Souptick Joarder 2016-02-17 0:08 ` Julian Calaby 2016-02-17 0:09 ` Julian Calaby 1 sibling, 1 reply; 7+ messages in thread From: Souptick Joarder @ 2016-02-16 6:36 UTC (permalink / raw) To: Alan; +Cc: linux-wireless, Kalle Valo On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: > The buffer needs to be zero terminated in case the user data is not. > Otherwise we run off the end of the buffer. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > --- > drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > index 25ee3cb..72ae530 100644 > --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ > { \ > struct rt2x00debug_intf *intf = file->private_data; \ > const struct rt2x00debug *debug = intf->debug; \ > - char line[16]; \ > + char line[17]; \ > size_t size; \ > unsigned int index = intf->offset_##__name; \ > __type value; \ > @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ > \ > if (copy_from_user(line, buf, length)) \ > return -EFAULT; \ > - \ > + line[16] = 0; line[length] = '\0'; correct me if I am wrong. \ > + \ > size = strlen(line); \ > value = simple_strtoul(line, NULL, 0); \ > \ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -Souptick ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-16 6:36 ` Souptick Joarder @ 2016-02-17 0:08 ` Julian Calaby 2016-02-17 13:49 ` Souptick Joarder 0 siblings, 1 reply; 7+ messages in thread From: Julian Calaby @ 2016-02-17 0:08 UTC (permalink / raw) To: Souptick Joarder; +Cc: Alan, linux-wireless, Kalle Valo Hi Souptick, On Tue, Feb 16, 2016 at 5:36 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: > On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: >> The buffer needs to be zero terminated in case the user data is not. >> Otherwise we run off the end of the buffer. >> >> Signed-off-by: Alan Cox <alan@linux.intel.com> >> --- >> drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >> index 25ee3cb..72ae530 100644 >> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >> @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >> { \ >> struct rt2x00debug_intf *intf = file->private_data; \ >> const struct rt2x00debug *debug = intf->debug; \ >> - char line[16]; \ >> + char line[17]; \ >> size_t size; \ >> unsigned int index = intf->offset_##__name; \ >> __type value; \ >> @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >> \ >> if (copy_from_user(line, buf, length)) \ >> return -EFAULT; \ >> - \ >> + line[16] = 0; > > line[length] = '\0'; > correct me if I am wrong. \ I believe that in this case the data in buf will already be null terminated, so ensuring that line is null terminated is only needed if there are exactly 16 bytes in buf. IMHO line[16] = 0; is dealing with this bug much more explicitly than line[length] = 0; however either will work. (BTW '\0' is identical to 0.) Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-17 0:08 ` Julian Calaby @ 2016-02-17 13:49 ` Souptick Joarder 2016-02-18 0:19 ` Julian Calaby 0 siblings, 1 reply; 7+ messages in thread From: Souptick Joarder @ 2016-02-17 13:49 UTC (permalink / raw) To: Julian Calaby; +Cc: Alan, linux-wireless, Kalle Valo Hi Julian, On Wed, Feb 17, 2016 at 5:38 AM, Julian Calaby <julian.calaby@gmail.com> wrote: > Hi Souptick, > > On Tue, Feb 16, 2016 at 5:36 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: >> On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: >>> The buffer needs to be zero terminated in case the user data is not. >>> Otherwise we run off the end of the buffer. >>> >>> Signed-off-by: Alan Cox <alan@linux.intel.com> >>> --- >>> drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>> index 25ee3cb..72ae530 100644 >>> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>> @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>> { \ >>> struct rt2x00debug_intf *intf = file->private_data; \ >>> const struct rt2x00debug *debug = intf->debug; \ >>> - char line[16]; \ >>> + char line[17]; \ >>> size_t size; \ >>> unsigned int index = intf->offset_##__name; \ >>> __type value; \ >>> @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>> \ >>> if (copy_from_user(line, buf, length)) \ >>> return -EFAULT; \ >>> - \ >>> + line[16] = 0; >> >> line[length] = '\0'; >> correct me if I am wrong. \ > > I believe that in this case the data in buf will already be null > terminated, so ensuring that line is null terminated is only needed if > there are exactly 16 bytes in buf. IMHO > > line[16] = 0; I think, if there are 16 bytes in buf, we end up loosing 1 byte data. If there are 15 bytes in buf, buf is already be null terminated. So is this really required? Correct me if I am wrong. > > is dealing with this bug much more explicitly than > > line[length] = 0; > > however either will work. (BTW '\0' is identical to 0.) > > Thanks, > > -- > Julian Calaby > > Email: julian.calaby@gmail.com > Profile: http://www.google.com/profiles/julian.calaby/ -Souptick ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-17 13:49 ` Souptick Joarder @ 2016-02-18 0:19 ` Julian Calaby 2016-02-18 3:43 ` Souptick Joarder 0 siblings, 1 reply; 7+ messages in thread From: Julian Calaby @ 2016-02-18 0:19 UTC (permalink / raw) To: Souptick Joarder; +Cc: Alan, linux-wireless, Kalle Valo Hi Souptick, On Thu, Feb 18, 2016 at 12:49 AM, Souptick Joarder <jrdr.linux@gmail.com> wrote: > Hi Julian, > > On Wed, Feb 17, 2016 at 5:38 AM, Julian Calaby <julian.calaby@gmail.com> wrote: >> Hi Souptick, >> >> On Tue, Feb 16, 2016 at 5:36 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: >>> On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: >>>> The buffer needs to be zero terminated in case the user data is not. >>>> Otherwise we run off the end of the buffer. >>>> >>>> Signed-off-by: Alan Cox <alan@linux.intel.com> >>>> --- >>>> drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>> index 25ee3cb..72ae530 100644 >>>> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>> @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>>> { \ >>>> struct rt2x00debug_intf *intf = file->private_data; \ >>>> const struct rt2x00debug *debug = intf->debug; \ >>>> - char line[16]; \ >>>> + char line[17]; \ >>>> size_t size; \ >>>> unsigned int index = intf->offset_##__name; \ >>>> __type value; \ >>>> @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>>> \ >>>> if (copy_from_user(line, buf, length)) \ >>>> return -EFAULT; \ >>>> - \ >>>> + line[16] = 0; >>> >>> line[length] = '\0'; >>> correct me if I am wrong. \ >> >> I believe that in this case the data in buf will already be null >> terminated, so ensuring that line is null terminated is only needed if >> there are exactly 16 bytes in buf. IMHO >> >> line[16] = 0; > > I think, if there are 16 bytes in buf, we end up loosing 1 byte data. > If there are 15 bytes in buf, buf is already be null terminated. > So is this really required? > Correct me if I am wrong. Counting in C starts at 0. We started out with line being a 16 character string, so it initially looked like this: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 <- index _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ <- data, i.e. line[index] If we are given a 4 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 a b c d \0 _ _ _ _ _ __ __ __ __ __ __ If we are given a 15 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 a b c d e f g h i j k l m n o \0 If we are given a 16 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 a b c d e f g h i j k l m n o p Note that it is not null terminated. The call to strlen() will now run off into memory that doesn't belong to line and potentially cause a crash. This is a bug. Alan fixed this by adding another character to the string, so it's now 17 characters long and line[16] will always be set to 0 after copy_from_user() is called. So now line initially looks like: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ __ If we are given a 4 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 a b c d \0 _ _ _ _ _ __ __ __ __ __ __ \0 If we are given a 15 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 a b c d e f g h i j k l m n o \0 \0 If we are given a 16 byte null terminated string, we end up with: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 a b c d e f g h i j k l m n o p \0 Note that the final null terminator is set by the line[16] = 0; line, not copy_from_user(). As line is now null terminated, strlen() will behave correctly and there is no bug. There are a few different ways to fix this bug. All of them will involve increasing the size of line to 17, and the exact details of how we ensure the null terminator is set can vary. Alan has chosen what is likely to be the smallest option in compiled code size, the fastest, and one of the most obvious options. Understanding how arrays and strings work in C is essential knowledge if you're going to submit or review patches for the Linux kernel. Could you please refresh your knowledge on those topics before submitting or reviewing any more patches? Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-18 0:19 ` Julian Calaby @ 2016-02-18 3:43 ` Souptick Joarder 0 siblings, 0 replies; 7+ messages in thread From: Souptick Joarder @ 2016-02-18 3:43 UTC (permalink / raw) To: Julian Calaby; +Cc: Alan, linux-wireless, Kalle Valo On Thu, Feb 18, 2016 at 5:49 AM, Julian Calaby <julian.calaby@gmail.com> wrote: > Hi Souptick, > > On Thu, Feb 18, 2016 at 12:49 AM, Souptick Joarder <jrdr.linux@gmail.com> wrote: >> Hi Julian, >> >> On Wed, Feb 17, 2016 at 5:38 AM, Julian Calaby <julian.calaby@gmail.com> wrote: >>> Hi Souptick, >>> >>> On Tue, Feb 16, 2016 at 5:36 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: >>>> On Tue, Feb 16, 2016 at 12:34 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: >>>>> The buffer needs to be zero terminated in case the user data is not. >>>>> Otherwise we run off the end of the buffer. >>>>> >>>>> Signed-off-by: Alan Cox <alan@linux.intel.com> >>>>> --- >>>>> drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>>> index 25ee3cb..72ae530 100644 >>>>> --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>>> +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c >>>>> @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>>>> { \ >>>>> struct rt2x00debug_intf *intf = file->private_data; \ >>>>> const struct rt2x00debug *debug = intf->debug; \ >>>>> - char line[16]; \ >>>>> + char line[17]; \ >>>>> size_t size; \ >>>>> unsigned int index = intf->offset_##__name; \ >>>>> __type value; \ >>>>> @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ >>>>> \ >>>>> if (copy_from_user(line, buf, length)) \ >>>>> return -EFAULT; \ >>>>> - \ >>>>> + line[16] = 0; >>>> >>>> line[length] = '\0'; >>>> correct me if I am wrong. \ >>> >>> I believe that in this case the data in buf will already be null >>> terminated, so ensuring that line is null terminated is only needed if >>> there are exactly 16 bytes in buf. IMHO >>> >>> line[16] = 0; >> >> I think, if there are 16 bytes in buf, we end up loosing 1 byte data. >> If there are 15 bytes in buf, buf is already be null terminated. >> So is this really required? >> Correct me if I am wrong. > > Counting in C starts at 0. > > We started out with line being a 16 character string, so it initially > looked like this: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 <- index > _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ <- data, i.e. line[index] > > If we are given a 4 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 > a b c d \0 _ _ _ _ _ __ __ __ __ __ __ > > If we are given a 15 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 > a b c d e f g h i j k l m n o \0 > > If we are given a 16 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 > a b c d e f g h i j k l m n o p > > Note that it is not null terminated. The call to strlen() will now run > off into memory that doesn't belong to line and potentially cause a > crash. This is a bug. > > Alan fixed this by adding another character to the string, so it's now > 17 characters long and line[16] will always be set to 0 after > copy_from_user() is called. So now line initially looks like: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 > _ _ _ _ _ _ _ _ _ _ __ __ __ __ __ __ __ > > If we are given a 4 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 > a b c d \0 _ _ _ _ _ __ __ __ __ __ __ \0 > > If we are given a 15 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 > a b c d e f g h i j k l m n o \0 \0 > > If we are given a 16 byte null terminated string, we end up with: > > 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 > a b c d e f g h i j k l m n o p \0 > > Note that the final null terminator is set by the line[16] = 0; line, > not copy_from_user(). > > As line is now null terminated, strlen() will behave correctly and > there is no bug. > > There are a few different ways to fix this bug. All of them will > involve increasing the size of line to 17, and the exact details of > how we ensure the null terminator is set can vary. Alan has chosen > what is likely to be the smallest option in compiled code size, the > fastest, and one of the most obvious options. > > Understanding how arrays and strings work in C is essential knowledge > if you're going to submit or review patches for the Linux kernel. > Could you please refresh your knowledge on those topics before > submitting or reviewing any more patches? Sorry, error from my side. :( Thanks for clarification. > Thanks, > > -- > Julian Calaby > > Email: julian.calaby@gmail.com > Profile: http://www.google.com/profiles/julian.calaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rt2x00: unterminated strlen of user data 2016-02-15 19:04 [PATCH] rt2x00: unterminated strlen of user data Alan 2016-02-16 6:36 ` Souptick Joarder @ 2016-02-17 0:09 ` Julian Calaby 1 sibling, 0 replies; 7+ messages in thread From: Julian Calaby @ 2016-02-17 0:09 UTC (permalink / raw) To: Alan; +Cc: linux-wireless, Kalle Valo Hi All, On Tue, Feb 16, 2016 at 6:04 AM, Alan <gnomes@lxorguk.ukuu.org.uk> wrote: > The buffer needs to be zero terminated in case the user data is not. > Otherwise we run off the end of the buffer. > > Signed-off-by: Alan Cox <alan@linux.intel.com> This looks right to me. Reviewed-by: Julian Calaby <julian.calaby@gmail.com> > --- > drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > index 25ee3cb..72ae530 100644 > --- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c > @@ -478,7 +478,7 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ > { \ > struct rt2x00debug_intf *intf = file->private_data; \ > const struct rt2x00debug *debug = intf->debug; \ > - char line[16]; \ > + char line[17]; \ > size_t size; \ > unsigned int index = intf->offset_##__name; \ > __type value; \ > @@ -494,7 +494,8 @@ static ssize_t rt2x00debug_write_##__name(struct file *file, \ > \ > if (copy_from_user(line, buf, length)) \ > return -EFAULT; \ > - \ > + line[16] = 0; \ > + \ > size = strlen(line); \ > value = simple_strtoul(line, NULL, 0); \ > \ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-18 3:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-15 19:04 [PATCH] rt2x00: unterminated strlen of user data Alan 2016-02-16 6:36 ` Souptick Joarder 2016-02-17 0:08 ` Julian Calaby 2016-02-17 13:49 ` Souptick Joarder 2016-02-18 0:19 ` Julian Calaby 2016-02-18 3:43 ` Souptick Joarder 2016-02-17 0:09 ` Julian Calaby
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox