From: isaacm@codeaurora.org
To: William Kucharski <william.kucharski@oracle.com>
Cc: David Laight <David.Laight@aculab.com>,
Kees Cook <keescook@chromium.org>,
crecklin@redhat.com, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, psodagud@codeaurora.org,
tsoni@codeaurora.org, stable@vger.kernel.org
Subject: Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
Date: Wed, 14 Nov 2018 09:32:50 -0800 [thread overview]
Message-ID: <50baa4900e55b523f18eea2759f8efae@codeaurora.org> (raw)
In-Reply-To: <7C54170F-DE66-47E0-9C0D-7D1A97DCD339@oracle.com>
On 2018-11-14 03:46, William Kucharski wrote:
>> On Nov 14, 2018, at 4:09 AM, David Laight <David.Laight@ACULAB.COM>
>> wrote:
>>
>> From: William Kucharski
>>> Sent: 14 November 2018 10:35
>>>
>>>> On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres
>>>> <isaacm@codeaurora.org> wrote:
>>>>
>>>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>>>> index 852eb4e..0293645 100644
>>>> --- a/mm/usercopy.c
>>>> +++ b/mm/usercopy.c
>>>> @@ -151,7 +151,7 @@ static inline void check_bogus_address(const
>>>> unsigned long ptr, unsigned long n,
>>>> bool to_user)
>>>> {
>>>> /* Reject if object wraps past end of memory. */
>>>> - if (ptr + n < ptr)
>>>> + if (ptr + (n - 1) < ptr)
>>>> usercopy_abort("wrapped address", NULL, to_user, 0, ptr + n);
>>>
>>> I'm being paranoid, but is it possible this routine could ever be
>>> passed "n" set to zero?
>>>
>>> If so, it will erroneously abort indicating a wrapped address as (n -
>>> 1) wraps to ULONG_MAX.
>>>
>>> Easily fixed via:
>>>
>>> if ((n != 0) && (ptr + (n - 1) < ptr))
>>
>> Ugg... you don't want a double test.
>>
>> I'd guess that a length of zero is likely, but a usercopy that
>> includes
>> the highest address is going to be invalid because it is a kernel
>> address
>> (on most archs, and probably illegal on others).
>> What you really want to do is add 'ptr + len' and check the carry
>> flag.
>
> The extra test is only a few extra instructions, but I understand the
> concern. (Though I don't
> know how you'd access the carry flag from C in a machine-independent
> way. Also, for the
> calculation to be correct you still need to check 'ptr + (len - 1)'
> for the wrap.)
>
> You could also theoretically call gcc's __builtin_uadd_overflow() if
> you want to get carried away.
>
> As I mentioned, I was just being paranoid, but the passed zero length
> issue stood out to me.
>
> William Kucharski
Hi William,
Thank you and David for your feedback. The check_bogus_address() routine
is only invoked from one place in the kernel, which is
__check_object_size(). Before invoking check_bogus_address,
__check_object_size ensures that n is non-zero, so it is not possible to
call this routine with n being 0. Therefore, we shouldn't run into the
scenario you described. Also, in the case where we are copying a page's
contents into a kernel space buffer and will not have that buffer
interacting with userspace at all, this change to that check should
still be valid, correct?
Thanks,
Isaac Manjarres
next prev parent reply other threads:[~2018-11-14 17:32 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-14 0:51 [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check Isaac J. Manjarres
2018-11-14 10:35 ` William Kucharski
2018-11-14 11:09 ` David Laight
2018-11-14 11:46 ` William Kucharski
2018-11-14 17:32 ` isaacm [this message]
2018-11-14 22:50 ` William Kucharski
2018-11-14 23:27 ` Kees Cook
2018-11-14 23:32 ` Kees Cook
2018-11-15 7:05 ` Sasha Levin
2019-07-30 17:54 Isaac J. Manjarres
2019-07-31 20:25 ` Kees Cook
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=50baa4900e55b523f18eea2759f8efae@codeaurora.org \
--to=isaacm@codeaurora.org \
--cc=David.Laight@aculab.com \
--cc=crecklin@redhat.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=psodagud@codeaurora.org \
--cc=stable@vger.kernel.org \
--cc=tsoni@codeaurora.org \
--cc=william.kucharski@oracle.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).