| Message ID | 20220902204351.2521805-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d07c0acb4f41cc42a0d97530946965b3e4fa68c1 |
| Headers | show |
| Series | Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y | expand |
On Fri, Sep 2, 2022 at 1:43 PM Kees Cook <keescook@chromium.org> wrote: > > Co-developed-by: Nick Desaulniers <ndesaulniers@google.com> That's overly generous of you! Anyways, the disassembly LGTM and the bot also came back green. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Android Treehugger Robot Link: https://android-review.googlesource.com/c/kernel/common/+/2206839 Another thought, Nikita suggested that you could also compare mode 1 vs mode 3: https://github.com/llvm/llvm-project/issues/57510#issuecomment-1235126343 That said, since mode 3 returns 0 for "unknown" I'd imagine that wouldn't be pretty since it wouldn't be a direct comparison against __p_size. Thanks again for the patch! > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/fortify-string.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index eed2119b23c5..07d5d1921eff 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -19,7 +19,8 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" > unsigned char *__p = (unsigned char *)(p); \ > size_t __ret = (size_t)-1; \ > size_t __p_size = __builtin_object_size(p, 1); \ > - if (__p_size != (size_t)-1) { \ > + if (__p_size != (size_t)-1 && \ > + __builtin_constant_p(*__p)) { \ > size_t __p_len = __p_size - 1; \ > if (__builtin_constant_p(__p[__p_len]) && \ > __p[__p_len] == '\0') \ > -- > 2.34.1 >
On Tue, Sep 06, 2022 at 07:36:46PM -0700, Nick Desaulniers wrote: > On Fri, Sep 2, 2022 at 1:43 PM Kees Cook <keescook@chromium.org> wrote: > > > > Co-developed-by: Nick Desaulniers <ndesaulniers@google.com> > > That's overly generous of you! Well, it was a lot of work to track down, and you wrote it up that way, I just moved things around a little bit. :) > Anyways, the disassembly LGTM and the bot also came back green. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > Tested-by: Android Treehugger Robot > Link: https://android-review.googlesource.com/c/kernel/common/+/2206839 Thank you! > Another thought, Nikita suggested that you could also compare mode 1 vs mode 3: > https://github.com/llvm/llvm-project/issues/57510#issuecomment-1235126343 Yeah, it could work (I tried this as well), but I think the better approach is checking index 0. > That said, since mode 3 returns 0 for "unknown" I'd imagine that > wouldn't be pretty since it wouldn't be a direct comparison against > __p_size. Yeah -- it is a little weird. I might come back to this if we get more glitches like this in the future.
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index eed2119b23c5..07d5d1921eff 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -19,7 +19,8 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" unsigned char *__p = (unsigned char *)(p); \ size_t __ret = (size_t)-1; \ size_t __p_size = __builtin_object_size(p, 1); \ - if (__p_size != (size_t)-1) { \ + if (__p_size != (size_t)-1 && \ + __builtin_constant_p(*__p)) { \ size_t __p_len = __p_size - 1; \ if (__builtin_constant_p(__p[__p_len]) && \ __p[__p_len] == '\0') \
With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we observe a runtime panic while running Android's Compatibility Test Suite's (CTS) android.hardware.input.cts.tests. This is stemming from a strlen() call in hidinput_allocate(). __compiletime_strlen() is implemented in terms of __builtin_object_size(), then does an array access to check for NUL-termination. A quirk of __builtin_object_size() is that for strings whose values are runtime dependent, __builtin_object_size(str, 1 or 0) returns the maximum size of possible values when those sizes are determinable at compile time. Example: static const char *v = "FOO BAR"; static const char *y = "FOO BA"; unsigned long x (int z) { // Returns 8, which is: // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1)) return __builtin_object_size(z ? v : y, 1); } So when FORTIFY_SOURCE is enabled, the current implementation of __compiletime_strlen() will try to access beyond the end of y at runtime using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault. hidinput_allocate() has a local C string whose value is control flow dependent on a switch statement, so __builtin_object_size(str, 1) evaluates to the maximum string length, making all other cases fault on the last character check. hidinput_allocate() could be cleaned up to avoid runtime calls to strlen() since the local variable can only have literal values, so there's no benefit to trying to fortify the strlen call site there. Perform a __builtin_constant_p() check against index 0 earlier in the macro to filter out the control-flow-dependant case. Add a KUnit test for checking the expected behavioral characteristics of FORTIFY_SOURCE internals. Cc: Nathan Chancellor <nathan@kernel.org> Cc: Tom Rix <trix@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: "Steven Rostedt (Google)" <rostedt@goodmis.org> Cc: David Gow <davidgow@google.com> Cc: Yury Norov <yury.norov@gmail.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Sander Vanheule <sander@svanheule.net> Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Co-developed-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/fortify-string.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)