From: Vegard Nossum <vegard.nossum@oracle.com>
To: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Dan Carpenter <dan.carpenter@oracle.com>,
"David S. Miller" <davem@davemloft.net>,
stable@vger.kernel.org
Subject: Re: [PATCH] isdnloop: NUL-terminate strings from userspace
Date: Tue, 01 Apr 2014 12:46:37 +0200 [thread overview]
Message-ID: <533A990D.2040609@oracle.com> (raw)
In-Reply-To: <20140401103020.GG24150@order.stressinduktion.org>
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
next prev parent reply other threads:[~2014-04-01 10:46 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-01 10:08 [PATCH] isdnloop: NUL-terminate strings from userspace Vegard Nossum
2014-04-01 10:30 ` Hannes Frederic Sowa
2014-04-01 10:46 ` Vegard Nossum [this message]
2014-04-01 11:02 ` Hannes Frederic Sowa
2014-04-01 12:35 ` Dan Carpenter
2014-04-01 20:18 ` David Miller
2014-04-02 3:48 ` [PATCH] isdnloop: Validate NUL-terminated strings from user YOSHIFUJI Hideaki
2014-04-03 15:27 ` David Miller
2014-04-02 8:47 ` [patch 1/2] lib/string.c: use the name "C-string" in comments Dan Carpenter
2014-04-02 8:47 ` [patch 2/2] lib/string.c: strlcpy() might read too far Dan Carpenter
2014-04-14 21:46 ` Andrew Morton
2014-04-15 10:38 ` Dan Carpenter
2014-04-14 22:21 ` Dave Jones
2014-04-01 11:23 ` [PATCH] isdnloop: NUL-terminate strings from userspace YOSHIFUJI Hideaki
-- strict thread matches above, loose matches on Subject: below --
2014-03-07 10:56 Vegard Nossum
2014-03-07 11:26 ` Dan Carpenter
2014-03-07 11:42 ` Vegard Nossum
2014-03-07 11:52 ` Dan Carpenter
2014-03-07 12:44 ` Vegard Nossum
2014-03-07 13:05 ` Dan Carpenter
2014-03-31 12:56 ` Vegard Nossum
2014-03-31 13:36 ` Dan Carpenter
2014-03-31 13:44 ` Vegard Nossum
2014-03-31 16:48 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=533A990D.2040609@oracle.com \
--to=vegard.nossum@oracle.com \
--cc=dan.carpenter@oracle.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.