[PATCH] isdnloop: NUL-terminate strings from userspace

[PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

Both the in-kernel and BSD strlcpy() require that the source string is
NUL terminated. We could use strncpy() + explicitly terminate the result,
but this relies on src and dest having the same size, so the safest thing
to do seems to explicitly terminate the source string before doing the
strlcpy().

Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 02125e6..50cd348 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;
 	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
 		return -EFAULT;
+
+	/*
+	 * Null terminate strings from userspace so we don't have to worry
+	 * about this later on.
+	 */
+	for (i = 0; i < 3; i++)
+		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
+
 	spin_lock_irqsave(&card->isdnloop_lock, flags);
 	switch (sdef.ptype) {
 	case ISDN_PTYPE_EURO:
-- 
1.7.10.4

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated.

No.  You're obviously wrong.  What on earth?

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
>> Both the in-kernel and BSD strlcpy() require that the source string is
>> NUL terminated.
>
> No.  You're obviously wrong.  What on earth?

Well, from lib/string.c:

size_t strlcpy(char *dest, const char *src, size_t size)
{
         size_t ret = strlen(src);

The BSD man page:

"Also note that strlcpy() and strlcat() only operate on true ``C'' 
strings.  This means that for strlcpy() src must be NUL-terminated and 
for strlcat() both src and dst must be NUL-terminated."


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
> On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> >On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> >>Both the in-kernel and BSD strlcpy() require that the source string is
> >>NUL terminated.
> >
> >No.  You're obviously wrong.  What on earth?
> 
> Well, from lib/string.c:
> 
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
>         size_t ret = strlen(src);
>

Ah...  So you mean that we could read far beyond the end of the string
and it would be a DoS because there would be 4 gigs of memory before we
hit a NUL character.  That won't happen in this case because the user
only controls a small buffer.  Normal memory is full of NUL chars.

I don't know the speed impact of changing the strlen() there to
strnlen().

> The BSD man page:
> 
> "Also note that strlcpy() and strlcat() only operate on true ``C''
> strings.  This means that for strlcpy() src must be NUL-terminated
> and for strlcat() both src and dst must be NUL-terminated."

It's talking about the kind of strings.  If it's a string which includes
NUL characters the strlcpy() won't work for that.  Or if it is not
*supposed* to end in a NUL character then it won't work for that.

We are using C strings here.

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 03/07/2014 12:52 PM, Dan Carpenter wrote:
> On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
>> On 03/07/2014 12:26 PM, Dan Carpenter wrote:
>>> On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
>>>> Both the in-kernel and BSD strlcpy() require that the source string is
>>>> NUL terminated.
>>>
>>> No.  You're obviously wrong.  What on earth?
>>
>> Well, from lib/string.c:
>>
>> size_t strlcpy(char *dest, const char *src, size_t size)
>> {
>>          size_t ret = strlen(src);
>>
>
> Ah...  So you mean that we could read far beyond the end of the string
> and it would be a DoS because there would be 4 gigs of memory before we
> hit a NUL character.  That won't happen in this case because the user
> only controls a small buffer.  Normal memory is full of NUL chars.

Well, that's true, but what happens if you accidentally read from an 
unmapped page or an mmio page?

I agree that it's not likely to happen in practice, but that's true for 
a lot of kernel bugs that we nevertheless want to fix. Are you saying 
that the patch is bad or wrong?

> I don't know the speed impact of changing the strlen() there to
> strnlen().

I don't think we could or should do that, because it changes the 
semantics of the function.  I think C string operations are probably 
confusing enough already without breaking compatibility with existing 
documentation and other implementations.

Also, the size parameter is the size of the destination buffer, not the 
source string. So even if we could make it safe to use strnlen() in this 
case, it still wouldn't be safe in other cases where the source string 
is shorter than the destination.

>
>> The BSD man page:
>>
>> "Also note that strlcpy() and strlcat() only operate on true ``C''
>> strings.  This means that for strlcpy() src must be NUL-terminated
>> and for strlcat() both src and dst must be NUL-terminated."
>
> It's talking about the kind of strings.  If it's a string which includes
> NUL characters the strlcpy() won't work for that.  Or if it is not
> *supposed* to end in a NUL character then it won't work for that.
>
> We are using C strings here.

I think the man page is quite clear: "for strlcpy() src must be 
NUL-terminated". Without the patch, this is not necessarily the case.


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Fri, Mar 07, 2014 at 01:44:17PM +0100, Vegard Nossum wrote:
> On 03/07/2014 12:52 PM, Dan Carpenter wrote:
> >On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
> >>On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> >>>On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> >>>>Both the in-kernel and BSD strlcpy() require that the source string is
> >>>>NUL terminated.
> >>>
> >>>No.  You're obviously wrong.  What on earth?
> >>
> >>Well, from lib/string.c:
> >>
> >>size_t strlcpy(char *dest, const char *src, size_t size)
> >>{
> >>         size_t ret = strlen(src);
> >>
> >
> >Ah...  So you mean that we could read far beyond the end of the string
> >and it would be a DoS because there would be 4 gigs of memory before we
> >hit a NUL character.  That won't happen in this case because the user
> >only controls a small buffer.  Normal memory is full of NUL chars.
> 
> Well, that's true, but what happens if you accidentally read from an
> unmapped page or an mmio page?

Interesting idea, but "sdef" is on the stack so that doesn't apply here
even though it might apply to other strlcpy() callers.

It might apply to other places.

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated. We could use strncpy() + explicitly terminate the result,
> but this relies on src and dest having the same size, so the safest thing
> to do seems to explicitly terminate the source string before doing the
> strlcpy().
>
> Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> index 02125e6..50cd348 100644
> --- a/drivers/isdn/isdnloop/isdnloop.c
> +++ b/drivers/isdn/isdnloop/isdnloop.c
> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>                 return -EBUSY;
>         if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>                 return -EFAULT;
> +
> +       /*
> +        * Null terminate strings from userspace so we don't have to worry
> +        * about this later on.
> +        */
> +       for (i = 0; i < 3; i++)
> +               sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
> +
>         spin_lock_irqsave(&card->isdnloop_lock, flags);
>         switch (sdef.ptype) {
>         case ISDN_PTYPE_EURO:

Ping, Dave? Just making sure this doesn't fall through the cracks. I
don't see the patch applied anywhere yet and without this patch we
still have a valid security concern IMO.


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Mon, Mar 31, 2014 at 02:56:07PM +0200, Vegard Nossum wrote:
> Ping, Dave? Just making sure this doesn't fall through the cracks. I
> don't see the patch applied anywhere yet and without this patch we
> still have a valid security concern IMO.

Gar.  No...  To recap:

> On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> > Both the in-kernel and BSD strlcpy() require that the source string is
> > NUL terminated.

The *whole point* of strlcpy() is that the source string doesn't have to
be NUL terminated.  The BSD man pages are trying to say that strlcpy()
only works on C-strings as opposed to Vstr or other safer string
implementions.

There is a potential problem in the kernel implementation of strlcpy()
because it does:

lib/string.c
   149  size_t strlcpy(char *dest, const char *src, size_t size)
   150  {
   151          size_t ret = strlen(src);
   152
   153          if (size) {
   154                  size_t len = (ret >= size) ? size - 1 : ret;
   155                  memcpy(dest, src, len);
   156                  dest[len] = '\0';
   157          }
   158          return ret;
   159  }

The strlen() on line 151 could read beyond the end of the source buffer
and if the memory wasn't mapped, it could Oops.

That concern doesn't apply here because the source string is on stack
memory and we will hit a NUL character before we hit unmapped memory.

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 31 March 2014 15:36, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Mar 31, 2014 at 02:56:07PM +0200, Vegard Nossum wrote:
>> Ping, Dave? Just making sure this doesn't fall through the cracks. I
>> don't see the patch applied anywhere yet and without this patch we
>> still have a valid security concern IMO.
>
> Gar.  No...  To recap:
>
>> On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> > Both the in-kernel and BSD strlcpy() require that the source string is
>> > NUL terminated.
>
> The *whole point* of strlcpy() is that the source string doesn't have to
> be NUL terminated.  The BSD man pages are trying to say that strlcpy()
> only works on C-strings as opposed to Vstr or other safer string
> implementions.

I read the BSD man page differently. Also, if you look at the actual
BSD implementation, it also scans the remaining buffer until it hits a
0.

I quote again: "for strlcpy() src must be NUL-terminated". It doesn't
get much clearer than that.

> There is a potential problem in the kernel implementation of strlcpy()
> because it does:
>
> lib/string.c
>    149  size_t strlcpy(char *dest, const char *src, size_t size)
>    150  {
>    151          size_t ret = strlen(src);
>    152
>    153          if (size) {
>    154                  size_t len = (ret >= size) ? size - 1 : ret;
>    155                  memcpy(dest, src, len);
>    156                  dest[len] = '\0';
>    157          }
>    158          return ret;
>    159  }
>
> The strlen() on line 151 could read beyond the end of the source buffer
> and if the memory wasn't mapped, it could Oops.
>
> That concern doesn't apply here because the source string is on stack
> memory and we will hit a NUL character before we hit unmapped memory.

As before, I agree that it's _likely_ we'll hit a 0 before hitting
unmapped memory, but I don't see at all that we have a guarantee of
it.


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[David Miller]

From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Mon, 31 Mar 2014 14:56:07 +0200

> Ping, Dave? Just making sure this doesn't fall through the cracks. I
> don't see the patch applied anywhere yet and without this patch we
> still have a valid security concern IMO.

If it's not sent to netdev, it's not in my queue.

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 1 Apr 2014 15:35:34 +0300

> I don't understand what you think the point of strlcpy() is, if it's not
> to deal with source strings which aren't NUL terminated.

If strlcpy() is meant to handle non-NULL terminated strings, then it's
kernel doc needs to be adjusted.

/**
 * strlcpy - Copy a %NUL terminated string into a sized buffer
 * @dest: Where to copy the string to
 * @src: Where to copy the string from
 * @size: size of destination buffer
 *
 * Compatible with *BSD: the result is always a valid
 * NUL-terminated string that fits in the buffer (unless,
 * of course, the buffer size is zero). It does not pad
 * out the result like strncpy() does.
 */

That says to me that 'src' is expected to be NULL terminated.

Furthermore, I like YOSHIFUJI Hideaki's idea that we should
actually validate the string and return -EINVAL if it is not
given to us NULL terminated.

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Tue, Apr 01, 2014 at 01:02:55PM +0200, Hannes Frederic Sowa wrote:
> On Tue, Apr 01, 2014 at 12:46:37PM +0200, Vegard Nossum wrote:
> > On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> > >Looking down the problem, it seems the problem is that the strlen in 
> > >strlcpy
> > >could read beyond the input buffer?
> > >
> > >To prevent this problem in other parts of the kernel wouldn't it be better 
> > >to
> > >replace the strlen with strnlen in strlcpy?
> > 
> > Sorry, I should have included the link to the previous thread: 
> > https://lkml.org/lkml/2014/3/7/712
> > 
> > I only resent (adding netdev to Cc) to get this into David Miller's 
> > patch queue.
> 
> Ah ok, sorry I don't follow lkml as closely as netdev@.
> 
> > As you can see from the previous discussion, we _could_ change the Linux 
> > kernel's definition of strlcpy(), but I wouldn't recommend it for the 
> > following reasons:
> > 
> > 1. Both BSD man page and BSD implementation _require_ the source string 
> > to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
> > kernel would probably be a bad idea and cause even more confusion that 
> > what we already have.
> 
> Sure, we shouldn't change the documented semantics. If at all it would
> be an additional safety net. Your patch would still be needed.
> 

Guys, really?  How would the patch "still be needed"?  I feel like if
someone said we had to rub a chicken head on this code we do it in the
name of security...

I don't understand what you think the point of strlcpy() is, if it's not
to deal with source strings which aren't NUL terminated.

I still maintain that the since the stack is full of NUL characters the
current implimentation of strlcpy() is ok for this isdn_loop function
and the patch is not needed at all without the strnlen() change.

However for other heap allocated variables then I could imagine that
the strlen() might be a problem.  I have two theories why we have never
seen problems with this in running code. 1) The string would have to be
at the end of a struct allocated at the end of a page.  You have to be
very unlucky to hit this requirement.  2) Most people pass valid data.

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[YOSHIFUJI Hideaki]

Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated. We could use strncpy() + explicitly terminate the result,
> but this relies on src and dest having the same size, so the safest thing
> to do seems to explicitly terminate the source string before doing the
> strlcpy().
:
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> index 02125e6..50cd348 100644
> --- a/drivers/isdn/isdnloop/isdnloop.c
> +++ b/drivers/isdn/isdnloop/isdnloop.c
> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>  		return -EBUSY;
>  	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>  		return -EFAULT;
> +
> +	/*
> +	 * Null terminate strings from userspace so we don't have to worry
> +	 * about this later on.
> +	 */
> +	for (i = 0; i < 3; i++)
> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
> +

Why don't we return -EINVAL if it is not correctly terminated by NUL?

--yoshfuji

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Hannes Frederic Sowa]

On Tue, Apr 01, 2014 at 12:46:37PM +0200, Vegard Nossum wrote:
> On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> >Looking down the problem, it seems the problem is that the strlen in 
> >strlcpy
> >could read beyond the input buffer?
> >
> >To prevent this problem in other parts of the kernel wouldn't it be better 
> >to
> >replace the strlen with strnlen in strlcpy?
> 
> Sorry, I should have included the link to the previous thread: 
> https://lkml.org/lkml/2014/3/7/712
> 
> I only resent (adding netdev to Cc) to get this into David Miller's 
> patch queue.

Ah ok, sorry I don't follow lkml as closely as netdev@.

> As you can see from the previous discussion, we _could_ change the Linux 
> kernel's definition of strlcpy(), but I wouldn't recommend it for the 
> following reasons:
> 
> 1. Both BSD man page and BSD implementation _require_ the source string 
> to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
> kernel would probably be a bad idea and cause even more confusion that 
> what we already have.

Sure, we shouldn't change the documented semantics. If at all it would
be an additional safety net. Your patch would still be needed.

> 2. Even if we changed strlcpy() to use strnlen(), it would still be 
> unsafe if the source string is not 0-terminated and the source buffer is 
> shorter than the destination buffer. That's because the size passed to 
> strlcpy() is conceptually the length of the _destination_ buffer, not 
> the source string.

Ack.

> I'm not against changing strlcpy() per se (changing to strnlen() might 
> be a performance improvement), but we shouldn't use that as an excuse to 
> use the interface incorrectly.

I am totally with you there.

Actually in some cases it could hinder finding such bugs as we're more
unlikely to hit a RED_ZONE which should crash the kernel (I actually
think crashes to find such bugs are good). But I guess the propability
is pretty high to hit another NUL byte before that and if at that point a
RED_ZONE is mapped.

Thanks,

  Hannes

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> On Tue, Apr 01, 2014 at 12:08:18PM +0200, Vegard Nossum wrote:
>> Both the in-kernel and BSD strlcpy() require that the source string is
>> NUL terminated. We could use strncpy() + explicitly terminate the result,
>> but this relies on src and dest having the same size, so the safest thing
>> to do seems to explicitly terminate the source string before doing the
>> strlcpy().
>>
>> Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>   drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
>> index 02125e6..50cd348 100644
>> --- a/drivers/isdn/isdnloop/isdnloop.c
>> +++ b/drivers/isdn/isdnloop/isdnloop.c
>> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>>   		return -EBUSY;
>>   	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>>   		return -EFAULT;
>> +
>> +	/*
>> +	 * Null terminate strings from userspace so we don't have to worry
>> +	 * about this later on.
>> +	 */
>> +	for (i = 0; i < 3; i++)
>> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
>> +
>
> Looking down the problem, it seems the problem is that the strlen in strlcpy
> could read beyond the input buffer?
>
> To prevent this problem in other parts of the kernel wouldn't it be better to
> replace the strlen with strnlen in strlcpy?

Sorry, I should have included the link to the previous thread: 
https://lkml.org/lkml/2014/3/7/712

I only resent (adding netdev to Cc) to get this into David Miller's 
patch queue.

As you can see from the previous discussion, we _could_ change the Linux 
kernel's definition of strlcpy(), but I wouldn't recommend it for the 
following reasons:

1. Both BSD man page and BSD implementation _require_ the source string 
to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
kernel would probably be a bad idea and cause even more confusion that 
what we already have.

2. Even if we changed strlcpy() to use strnlen(), it would still be 
unsafe if the source string is not 0-terminated and the source buffer is 
shorter than the destination buffer. That's because the size passed to 
strlcpy() is conceptually the length of the _destination_ buffer, not 
the source string.

I'm not against changing strlcpy() per se (changing to strnlen() might 
be a performance improvement), but we shouldn't use that as an excuse to 
use the interface incorrectly.


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Hannes Frederic Sowa]

On Tue, Apr 01, 2014 at 12:08:18PM +0200, Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated. We could use strncpy() + explicitly terminate the result,
> but this relies on src and dest having the same size, so the safest thing
> to do seems to explicitly terminate the source string before doing the
> strlcpy().
> 
> Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> index 02125e6..50cd348 100644
> --- a/drivers/isdn/isdnloop/isdnloop.c
> +++ b/drivers/isdn/isdnloop/isdnloop.c
> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>  		return -EBUSY;
>  	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>  		return -EFAULT;
> +
> +	/*
> +	 * Null terminate strings from userspace so we don't have to worry
> +	 * about this later on.
> +	 */
> +	for (i = 0; i < 3; i++)
> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
> +

Looking down the problem, it seems the problem is that the strlen in strlcpy
could read beyond the input buffer?

To prevent this problem in other parts of the kernel wouldn't it be better to
replace the strlen with strnlen in strlcpy?

Bye,

  Hannes

[PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

Both the in-kernel and BSD strlcpy() require that the source string is
NUL terminated. We could use strncpy() + explicitly terminate the result,
but this relies on src and dest having the same size, so the safest thing
to do seems to explicitly terminate the source string before doing the
strlcpy().

Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 02125e6..50cd348 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;
 	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
 		return -EFAULT;
+
+	/*
+	 * Null terminate strings from userspace so we don't have to worry
+	 * about this later on.
+	 */
+	for (i = 0; i < 3; i++)
+		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
+
 	spin_lock_irqsave(&card->isdnloop_lock, flags);
 	switch (sdef.ptype) {
 	case ISDN_PTYPE_EURO:
-- 
1.7.10.4