Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
On Tue, Jul 30, 2019 at 10:54:13AM -0700, Isaac J. Manjarres wrote: > Currently, when checking to see if accessing n bytes starting at > address "ptr" will cause a wraparound in the memory addresses, > the check in check_bogus_address() adds an extra byte, which is > incorrect, as the range of addresses that will be accessed is > [ptr, ptr + (n - 1)]. > > This can lead to incorrectly detecting a wraparound in the > memory address, when trying to read 4 KB from memory that is > mapped to the the last possible page in the virtual address > space, when in fact, accessing that range of memory would not > cause a wraparound to occur. > > Use the memory range that will actually be accessed when > considering if accessing a certain amount of bytes will cause > the memory address to wrap around. > > Fixes: f5509cc18daa ("mm: Hardened usercopy") > Co-developed-by: Prasad Sodagudi > Signed-off-by: Prasad Sodagudi > Signed-off-by: Isaac J. Manjarres > Cc: sta...@vger.kernel.org > Reviewed-by: William Kucharski > Acked-by: Kees Cook Ah, thanks for the reminder! (I got surprised by seeing my Ack in this email -- next time please use "v2" or "RESEND" to jog my memory.) This got lost last year; my bad. Andrew, can you take this or should I send it directly to Linus? -Kees > --- > mm/usercopy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/usercopy.c b/mm/usercopy.c > index 2a09796..98e92486 100644 > --- a/mm/usercopy.c > +++ b/mm/usercopy.c > @@ -147,7 +147,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); > > /* Reject if NULL or ZERO-allocation. */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Kees Cook
[PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
Currently, when checking to see if accessing n bytes starting at address "ptr" will cause a wraparound in the memory addresses, the check in check_bogus_address() adds an extra byte, which is incorrect, as the range of addresses that will be accessed is [ptr, ptr + (n - 1)]. This can lead to incorrectly detecting a wraparound in the memory address, when trying to read 4 KB from memory that is mapped to the the last possible page in the virtual address space, when in fact, accessing that range of memory would not cause a wraparound to occur. Use the memory range that will actually be accessed when considering if accessing a certain amount of bytes will cause the memory address to wrap around. Fixes: f5509cc18daa ("mm: Hardened usercopy") Co-developed-by: Prasad Sodagudi Signed-off-by: Prasad Sodagudi Signed-off-by: Isaac J. Manjarres Cc: sta...@vger.kernel.org Reviewed-by: William Kucharski Acked-by: Kees Cook --- mm/usercopy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/usercopy.c b/mm/usercopy.c index 2a09796..98e92486 100644 --- a/mm/usercopy.c +++ b/mm/usercopy.c @@ -147,7 +147,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); /* Reject if NULL or ZERO-allocation. */ -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
On Tue, Nov 13, 2018 at 6:51 PM, Isaac J. Manjarres wrote: > Currently, when checking to see if accessing n bytes starting at > address "ptr" will cause a wraparound in the memory addresses, > the check in check_bogus_address() adds an extra byte, which is > incorrect, as the range of addresses that will be accessed is > [ptr, ptr + (n - 1)]. > > This can lead to incorrectly detecting a wraparound in the > memory address, when trying to read 4 KB from memory that is > mapped to the the last possible page in the virtual address > space, when in fact, accessing that range of memory would not > cause a wraparound to occur. I'm kind of surprised anything is using the -4K memory range -- this is ERR_PTR() area and I'd expect there to be an explicit unallocated memory hole here. > > Use the memory range that will actually be accessed when > considering if accessing a certain amount of bytes will cause > the memory address to wrap around. > > Change-Id: I2563a5988e41122727ede17180f365e999b953e6 > Fixes: f5509cc18daa ("mm: Hardened usercopy") > Co-Developed-by: Prasad Sodagudi > Signed-off-by: Prasad Sodagudi > Signed-off-by: Isaac J. Manjarres > Cc: sta...@vger.kernel.org Regardless, I'll take it in my tree if akpm doesn't grab it first. :) Acked-by: Kees Cook -Kees > --- > mm/usercopy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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); > > /* Reject if NULL or ZERO-allocation. */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Kees Cook
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
On Wed, Nov 14, 2018 at 4:35 AM, William Kucharski wrote: > > >> On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres >> 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? It's a single-use inline, and zero is tested just before getting called: /* Skip all tests if size is zero. */ if (!n) return; /* Check for invalid addresses. */ check_bogus_address((const unsigned long)ptr, n, to_user); > > 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)) Agreed. Thanks for noticing this! -Kees -- Kees Cook
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
> On Nov 14, 2018, at 10:32 AM, isa...@codeaurora.org wrote: > > 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? Having fixed more than one bug resulting from a "only called in one place" routine later being called elsewhere, I am wary, but ultimately it's likely not worth the performance hit of a check or BUG_ON(). It's a generic math check for overflow, so it should work with any address. Reviewed-by: William Kucharski
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
On 2018-11-14 03:46, William Kucharski wrote: On Nov 14, 2018, at 4:09 AM, David Laight wrote: From: William Kucharski Sent: 14 November 2018 10:35 On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres 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
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
> On Nov 14, 2018, at 4:09 AM, David Laight wrote: > > From: William Kucharski >> Sent: 14 November 2018 10:35 >> >>> On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres >>> 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
RE: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
From: William Kucharski > Sent: 14 November 2018 10:35 > > > On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres > > 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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
> On Nov 13, 2018, at 5:51 PM, Isaac J. Manjarres 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)) William Kucharski
[PATCH] mm/usercopy: Use memory range to be accessed for wraparound check
Currently, when checking to see if accessing n bytes starting at address "ptr" will cause a wraparound in the memory addresses, the check in check_bogus_address() adds an extra byte, which is incorrect, as the range of addresses that will be accessed is [ptr, ptr + (n - 1)]. This can lead to incorrectly detecting a wraparound in the memory address, when trying to read 4 KB from memory that is mapped to the the last possible page in the virtual address space, when in fact, accessing that range of memory would not cause a wraparound to occur. Use the memory range that will actually be accessed when considering if accessing a certain amount of bytes will cause the memory address to wrap around. Change-Id: I2563a5988e41122727ede17180f365e999b953e6 Fixes: f5509cc18daa ("mm: Hardened usercopy") Co-Developed-by: Prasad Sodagudi Signed-off-by: Prasad Sodagudi Signed-off-by: Isaac J. Manjarres Cc: sta...@vger.kernel.org --- mm/usercopy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); /* Reject if NULL or ZERO-allocation. */ -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project