Re: [PATCH] isdnloop: NUL-terminate strings from userspace
From: Dan Carpenter 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 Cc: David S. Miller Cc: sta...@vger.kernel.org Signed-off-by: Vegard Nossum --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 > Cc: David S. Miller > Cc: sta...@vger.kernel.org > Signed-off-by: Vegard Nossum > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] isdnloop: NUL-terminate strings from userspace
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 Cc: David S. Miller Cc: sta...@vger.kernel.org Signed-off-by: Vegard Nossum --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
From: Vegard Nossum 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
On 31 March 2014 15:36, Dan Carpenter 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
On 7 March 2014 11:56, 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 > Cc: David S. Miller > Cc: sta...@vger.kernel.org > Signed-off-by: Vegard Nossum > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] isdnloop: NUL-terminate strings from userspace
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] isdnloop: NUL-terminate strings from userspace
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 Cc: David S. Miller Cc: sta...@vger.kernel.org Signed-off-by: Vegard Nossum --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/