| Message ID | 1478106169-25770-1-git-send-email-vaishali.thakkar@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote: > The routine csum_partial_copy_from_user is same as csum_partial_copy > but it copies from user space for the checksumming. In other respects > it is identical, and can be used to copy an arbitrarily large buffer > from userspace into the kernel. Conceptually this exposes a similar > attack surface like copy_from_user. So, to validate the given address > we should call check_object_size here. Thanks for looking at this! I agree that we should be trying lock down these homebrew/specialised copy_{to,from}_user routines. However... > @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len, > { > int missing; > > + check_object_size(dst, len, false); > missing = __copy_from_user(dst, src, len); ... here we're just calling into the architecture-specific __copy_from_user(), and I know that both arm64 and x86 have a check_object_size() call in their __copy_from_user() implementations. Is that missing on some architectures? I think we need to figure out where check_object_size() and other checks (e.g. kasan_check_size) are expected to live in the hierarchy of uaccess copy primitives (and/or if they should also live in {get,put)_user()). I had a plan to try to refactor the generic uaccess code so that we could put those checks in one place, but I put that on hold as Al Viro was doing some overlapping refactoring of all the uaccess primitives (and I got busy with some other work). Thanks, Mark.
On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote: >> The routine csum_partial_copy_from_user is same as csum_partial_copy >> but it copies from user space for the checksumming. In other respects >> it is identical, and can be used to copy an arbitrarily large buffer >> from userspace into the kernel. Conceptually this exposes a similar >> attack surface like copy_from_user. So, to validate the given address >> we should call check_object_size here. > > Thanks for looking at this! I agree that we should be trying lock down these > homebrew/specialised copy_{to,from}_user routines. Yes, I'm glad to have more eyes on this. I did notice that these are almost exclusively used in networking. I'd be curious to see how noticeable this is on performance (though see my inlining comment below...) > > However... > >> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len, >> { >> int missing; >> >> + check_object_size(dst, len, false); >> missing = __copy_from_user(dst, src, len); > > ... here we're just calling into the architecture-specific __copy_from_user(), > and I know that both arm64 and x86 have a check_object_size() call in their > __copy_from_user() implementations. Another issue here is that csum_partial_copy_from_user() isn't an inline function, so we lose the performance benefits of ignoring builtin_cost uses. > Is that missing on some architectures? Every architecture is _slightly_ different. Most put the check in __copy_from_user() so it's correctly caught. (x86 puts them in both since copy*() calls _copy*(), not __copy*() ... >_<) > I think we need to figure out where check_object_size() and other checks (e.g. > kasan_check_size) are expected to live in the hierarchy of uaccess copy > primitives (and/or if they should also live in {get,put)_user()). Yes, totally agreed. This is going to be needed for the next step of usercopy hardening, too. We need to have an arch-specific __actually_do_the_copy() function that has none of the instrumentation and is only visible to the usercopy functions. Then we can separate check logic from the action itself. (Right now we can't pass any additional information to the check (e.g. whether to make exceptions to slab whitelisting, etc) because the check is deep in __copy*user(). > I had a plan to try to refactor the generic uaccess code so that we could put > those checks in one place, but I put that on hold as Al Viro was doing some > overlapping refactoring of all the uaccess primitives (and I got busy with some > other work). Al, is your pass at improving uaccess currently finished, or do you have more changes coming? -Kees
On Thursday 03 November 2016 03:29 AM, Kees Cook wrote: > On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> Hi, >> >> On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote: >>> The routine csum_partial_copy_from_user is same as csum_partial_copy >>> but it copies from user space for the checksumming. In other respects >>> it is identical, and can be used to copy an arbitrarily large buffer >>> from userspace into the kernel. Conceptually this exposes a similar >>> attack surface like copy_from_user. So, to validate the given address >>> we should call check_object_size here. >> >> Thanks for looking at this! I agree that we should be trying lock down these >> homebrew/specialised copy_{to,from}_user routines. > > Yes, I'm glad to have more eyes on this. I did notice that these are > almost exclusively used in networking. I'd be curious to see how > noticeable this is on performance (though see my inlining comment > below...) > >> >> However... >> >>> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len, >>> { >>> int missing; >>> >>> + check_object_size(dst, len, false); >>> missing = __copy_from_user(dst, src, len); >> >> ... here we're just calling into the architecture-specific __copy_from_user(), >> and I know that both arm64 and x86 have a check_object_size() call in their >> __copy_from_user() implementations. > > Another issue here is that csum_partial_copy_from_user() isn't an > inline function, so we lose the performance benefits of ignoring > builtin_cost uses. > >> Is that missing on some architectures? > > Every architecture is _slightly_ different. Most put the check in > __copy_from_user() so it's correctly caught. (x86 puts them in both > since copy*() calls _copy*(), not __copy*() ... >_<) I think still there are some architectures which didn't put the check in __copy_from_user() [eg. tile]. >> I think we need to figure out where check_object_size() and other checks (e.g. >> kasan_check_size) are expected to live in the hierarchy of uaccess copy >> primitives (and/or if they should also live in {get,put)_user()). > > Yes, totally agreed. This is going to be needed for the next step of > usercopy hardening, too. We need to have an arch-specific > __actually_do_the_copy() function that has none of the instrumentation > and is only visible to the usercopy functions. Then we can separate > check logic from the action itself. (Right now we can't pass any > additional information to the check (e.g. whether to make exceptions > to slab whitelisting, etc) because the check is deep in __copy*user(). I was actually wondering if there are any cases where we need any architecture specific extra check(s)? >> I had a plan to try to refactor the generic uaccess code so that we could put >> those checks in one place, but I put that on hold as Al Viro was doing some >> overlapping refactoring of all the uaccess primitives (and I got busy with some >> other work). > > Al, is your pass at improving uaccess currently finished, or do you > have more changes coming? > > -Kees >
Hi Vaishali, On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote: > On Thursday 03 November 2016 03:29 AM, Kees Cook wrote: > > On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >> I know that both arm64 and x86 have a check_object_size() call in their > >> __copy_from_user() implementations. > >> Is that missing on some architectures? > > > > Every architecture is _slightly_ different. Most put the check in > > __copy_from_user() so it's correctly caught. (x86 puts them in both > > since copy*() calls _copy*(), not __copy*() ... >_<) > > I think still there are some architectures which didn't put the check > in __copy_from_user() [eg. tile]. I see. :( Looking again, a grep shows many (even those with MMUs) don't do anything at all in v4.9-rc2: [mark@remoulade:~/src/linux]% for ARCH in arch/*; do printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH}; done | sort -n 0 arch/alpha 0 arch/arc 0 arch/avr32 0 arch/blackfin 0 arch/c6x 0 arch/cris 0 arch/frv 0 arch/h8300 0 arch/hexagon 0 arch/Kconfig 0 arch/m32r 0 arch/m68k 0 arch/metag 0 arch/microblaze 0 arch/mn10300 0 arch/nios2 0 arch/openrisc 0 arch/score 0 arch/sh 0 arch/tile 0 arch/um 0 arch/unicore32 0 arch/xtensa 2 arch/parisc 2 arch/s390 3 arch/arm 4 arch/arm64 4 arch/ia64 4 arch/powerpc 5 arch/sparc 6 arch/mips 6 arch/x86 > I was actually wondering if there are any cases where we need any > architecture specific extra check(s)? Generally, I'd expect that to be orthogonal to the hardened usercopy work, and that check would still be present in the low-level architecture-specific code even if we made the check_object_size() checks common. Do you have an example of the kind of thing you're worried about? Thanks, Mark.
On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote: > Hi Vaishali, Hi Mark, > On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote: >> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote: >>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>>> I know that both arm64 and x86 have a check_object_size() call in their >>>> __copy_from_user() implementations. > >>>> Is that missing on some architectures? >>> >>> Every architecture is _slightly_ different. Most put the check in >>> __copy_from_user() so it's correctly caught. (x86 puts them in both >>> since copy*() calls _copy*(), not __copy*() ... >_<) >> >> I think still there are some architectures which didn't put the check >> in __copy_from_user() [eg. tile]. > > I see. :( > > Looking again, a grep shows many (even those with MMUs) don't do anything at > all in v4.9-rc2: > > [mark@remoulade:~/src/linux]% for ARCH in arch/*; do > printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH}; > done | sort -n > 0 arch/alpha > 0 arch/arc > 0 arch/avr32 > 0 arch/blackfin > 0 arch/c6x > 0 arch/cris > 0 arch/frv > 0 arch/h8300 > 0 arch/hexagon > 0 arch/Kconfig > 0 arch/m32r > 0 arch/m68k > 0 arch/metag > 0 arch/microblaze > 0 arch/mn10300 > 0 arch/nios2 > 0 arch/openrisc > 0 arch/score > 0 arch/sh > 0 arch/tile > 0 arch/um > 0 arch/unicore32 > 0 arch/xtensa > 2 arch/parisc > 2 arch/s390 > 3 arch/arm > 4 arch/arm64 > 4 arch/ia64 > 4 arch/powerpc > 5 arch/sparc > 6 arch/mips > 6 arch/x86 Hmm, should we go for sending patches for them? [atleast for the ones with MMUs and then may be maintainers/developers can check the change] Also, I think same goes for the the kasan_check. We have only arm64 and x86 with these checks. >> I was actually wondering if there are any cases where we need any >> architecture specific extra check(s)? > > Generally, I'd expect that to be orthogonal to the hardened usercopy work, and > that check would still be present in the low-level architecture-specific code > even if we made the check_object_size() checks common. > > Do you have an example of the kind of thing you're worried about? Ahmm, no. Not at the moment. But I was actually thinking that this might be possible based on how architecture copies data from the userspace. Probably this can be figure out later on by architecture maintainers once they have this check ... > Thanks, > Mark. >
On Wed, Nov 02, 2016 at 03:59:25PM -0600, Kees Cook wrote: > On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > > > On Wed, Nov 02, 2016 at 10:32:49PM +0530, Vaishali Thakkar wrote: > >> The routine csum_partial_copy_from_user is same as csum_partial_copy > >> but it copies from user space for the checksumming. In other respects > >> it is identical, and can be used to copy an arbitrarily large buffer > >> from userspace into the kernel. Conceptually this exposes a similar > >> attack surface like copy_from_user. So, to validate the given address > >> we should call check_object_size here. > > > > Thanks for looking at this! I agree that we should be trying lock down these > > homebrew/specialised copy_{to,from}_user routines. > > Yes, I'm glad to have more eyes on this. I did notice that these are > almost exclusively used in networking. I'd be curious to see how > noticeable this is on performance (though see my inlining comment > below...) No check_object_size() in that place. Really. Look through the call chains. > Al, is your pass at improving uaccess currently finished, or do you > have more changes coming? Yes, and quite a few. There's going to be a lot of merging between the architectures in that area. As for csum stuff... csum_partial_copy_from_user() is *NOT* an exposed API. Really. It's a helper often used by implementations of csum_partial_copy_nocheck() and csum_and_copy_from_user(). Those two are parts of API. With very limited use. csum_and_copy_from_user() has literally only one user - csum_and_copy_from_iter(). csum_partial_copy_nocheck() has slightly wider use. It has nothing to do with userland memory, though, and should never be used for such. First of all, it's used in implementation of csum_and_copy_{to,from}_iter(). Then there are * skb_copy_and_csum_bits() * getfrag callbacks of ip_append_data() - icmp_push_reply(), ip_reply_glue_bits(), raw_getfrag(), raw6_getfrag() * tcp_fragment() * really weird shit in drivers/net/ethernet/3com/typhoon.c - the hardware apparently uses IP-style csum as a signature for the firmware. Yes, really. Comments regarding the value of such 'protection' (whatever it was they were trying to protect) are best directed to 3COM people. What would you check the sizes of? The most common users are csum_and_copy_from_iter() and skb_copy_and_csum_bits(). And serious overhead added to either of those will be _very_ painful.
On Thu, Nov 03, 2016 at 10:26:41AM +0530, Vaishali Thakkar wrote: > On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote: > > On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote: > >> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote: > >>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: > >>>> I know that both arm64 and x86 have a check_object_size() call in their > >>>> __copy_from_user() implementations. > > > >>>> Is that missing on some architectures? > > Looking again, a grep shows many (even those with MMUs) don't do anything at > > all in v4.9-rc2: > > > > [mark@remoulade:~/src/linux]% for ARCH in arch/*; do > > printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH}; > > done | sort -n > Hmm, should we go for sending patches for them? [atleast for the ones with MMUs > and then may be maintainers/developers can check the change] If Al's uaccess unification work is arriving shortly, sending patches for those in parallel is just going to make matters more painful. So it really depends on when that's likely to appear. > Also, I think same goes for the the kasan_check. We have only arm64 and x86 > with these checks. Yes. I'd hoped to collect all of those behind a common helper, something like: static inline void check_uaccess_read(void *kaddr, const void __user *uaddr, unsigned long n) { kasan_check_write(kaddr, n); check_object_size(kaddr, n, false); any_uaddr_sanity_check(uaddr, n); } Thanks, Mark.
On Thursday 03 November 2016 11:35 PM, Mark Rutland wrote: > On Thu, Nov 03, 2016 at 10:26:41AM +0530, Vaishali Thakkar wrote: >> On Thursday 03 November 2016 09:53 AM, Mark Rutland wrote: >>> On Thu, Nov 03, 2016 at 07:44:35AM +0530, Vaishali Thakkar wrote: >>>> On Thursday 03 November 2016 03:29 AM, Kees Cook wrote: >>>>> On Wed, Nov 2, 2016 at 2:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>>>>> I know that both arm64 and x86 have a check_object_size() call in their >>>>>> __copy_from_user() implementations. >>> >>>>>> Is that missing on some architectures? > >>> Looking again, a grep shows many (even those with MMUs) don't do anything at >>> all in v4.9-rc2: >>> >>> [mark@remoulade:~/src/linux]% for ARCH in arch/*; do >>> printf "%d %s\n" $(git grep check_object_size -- "${ARCH}" | wc -l) ${ARCH}; >>> done | sort -n > >> Hmm, should we go for sending patches for them? [atleast for the ones with MMUs >> and then may be maintainers/developers can check the change] > > If Al's uaccess unification work is arriving shortly, sending patches for those > in parallel is just going to make matters more painful. > > So it really depends on when that's likely to appear. Makes sense. >> Also, I think same goes for the the kasan_check. We have only arm64 and x86 >> with these checks. > > Yes. I'd hoped to collect all of those behind a common helper, something like: > > static inline void check_uaccess_read(void *kaddr, const void __user *uaddr, unsigned long n) > { > kasan_check_write(kaddr, n); > check_object_size(kaddr, n, false); > any_uaddr_sanity_check(uaddr, n); > } Ok, sounds reasonable. Thanks. > Thanks, > Mark. >
diff --git a/lib/checksum.c b/lib/checksum.c index d3ec93f..2e0fec8 100644 --- a/lib/checksum.c +++ b/lib/checksum.c @@ -33,6 +33,7 @@ kills, so most of the assembly has to go. */ #include <linux/export.h> +#include <linux/thread_info.h> #include <net/checksum.h> #include <asm/byteorder.h> @@ -158,6 +159,7 @@ csum_partial_copy_from_user(const void __user *src, void *dst, int len, { int missing; + check_object_size(dst, len, false); missing = __copy_from_user(dst, src, len); if (missing) { memset(dst + len - missing, 0, missing);
The routine csum_partial_copy_from_user is same as csum_partial_copy but it copies from user space for the checksumming. In other respects it is identical, and can be used to copy an arbitrarily large buffer from userspace into the kernel. Conceptually this exposes a similar attack surface like copy_from_user. So, to validate the given address we should call check_object_size here. Note that in the absence of hardened usercopy this will have no impact. Signed-off-by: Vaishali Thakkar <vaishali.thakkar@oracle.com> --- lib/checksum.c | 2 ++ 1 file changed, 2 insertions(+)