mm/usercopy: Use memory range to be accessed for wraparound check
PATCH 1/1 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.
--- mm/usercopy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/usercopy.c +1 -1
@@ -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. */
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 9 lines · Isaac J. Manjarres
mm/usercopy.c +0
-0
mm/usercopy.c static inline void check_bogus_address(const unsigned long ptr, unsigned long n, · line 155
Quoted 6 lines
mm/usercopy.c +1
-1@@ -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
From: William Kucharski
Quoted 18 lines
Sent: 14 November 2018 10:35
mm/usercopy.c static inline void check_bogus_address(const unsigned long ptr, unsigned long n, · line 155
Quoted 13 lines · Isaac J. Manjarres
mm/usercopy.c +1
-1@@ -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?
Quoted 5 lines
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)
Quoted 27 lines · David Laight
From: William Kucharski
Quoted 22 lines
Sent: 14 November 2018 10:35
mm/usercopy.c static inline void check_bogus_address(const unsigned long ptr, unsigned long n, · line 155
Quoted 13 lines · Isaac J. Manjarres
mm/usercopy.c +1
-1@@ -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:
Quoted 8 lines
Quoted 1 line
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
Quoted 42 lines · William Kucharski
Quoted 26 lines · David Laight
From: William Kucharski
Quoted 23 lines
Sent: 14 November 2018 10:35
Quoted 7 lines · Isaac J. Manjarres
mm/usercopy.c +0
-0mm/usercopy.c static inline void check_bogus_address(const · line 156
Quoted 7 lines
mm/usercopy.c +1
-1@@ -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:
Quoted 8 lines
Quoted 1 line
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.)
Quoted 7 lines
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
William Kucharski
Reviewed-byQuoted 5 lines · On Nov 14, 2018, at 10:32 AM, [email protected] 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 invokingcheck_bogus_address,__check_object_sizeensures 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.
Quoted 4 lines · William Kucharski
Quoted 4 lines · Isaac J. Manjarres
Quoted 13 lines
mm/usercopy.c static inline void check_bogus_address(const unsigned long ptr, unsigned long n, · line 155
Quoted 11 lines
mm/usercopy.c +1
-1@@ -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);Quoted 6 lines
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
Signature · 1 line
Kees Cook
Quoted 11 lines · Isaac J. Manjarres
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.
Quoted 11 lines
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 I2563a5988e41122727ede17180f365e999b953e6Fixes f5509cc18daa ("mm: Hardened usercopy")Co-Developed-by Prasad Sodagudi <[email protected]>Signed-off-by Prasad Sodagudi <[email protected]>Signed-off-by Isaac J. Manjarres <[email protected]>
Regardless, I'll take it in my tree if akpm doesn't grab it first. :)
-Kees
Quoted 21 lines
--- mm/usercopy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/usercopy.c +1
-2@@ -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
Signature · 1 line
Kees Cook
Sasha Levin
maintainerHi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag, fixing commit: f5509cc18daa mm: Hardened usercopy.
The bot has tested the following trees: v4.19.2, v4.18.19, v4.14.81, v4.9.137.
v4.19.2: Build OK!
v4.18.19: Build OK!
v4.14.81: Failed to apply! Possible dependencies:
Unable to calculate
v4.9.137: Failed to apply! Possible dependencies:
Unable to calculate
--
Thanks,
Sasha