| Message ID | 20231121220155.1217090-14-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kmsan: Enable on s390 | expand |
On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > Add a wrapper for memset() that prevents unpoisoning. We have __memset() already, won't it work for this case? On the other hand, I am not sure you want to preserve the redzone in its previous state (unless it's known to be poisoned). You might consider explicitly unpoisoning the redzone instead. ... > +__no_sanitize_memory > +static inline void *memset_no_sanitize_memory(void *s, int c, size_t n) > +{ > + return memset(s, c, n); > +} I think depending on the compiler optimizations this might end up being a call to normal memset, that would still change the shadow bytes.
On Fri, 2023-12-08 at 14:48 +0100, Alexander Potapenko wrote: > On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > Add a wrapper for memset() that prevents unpoisoning. > > We have __memset() already, won't it work for this case? A problem with __memset() is that, at least for me, it always ends up being a call. There is a use case where we need to write only 1 byte, so I thought that introducing a call there (when compiling without KMSAN) would be unacceptable. > On the other hand, I am not sure you want to preserve the redzone in > its previous state (unless it's known to be poisoned). That's exactly the problem with unpoisoning: it removes the distinction between a new allocation and a UAF. > You might consider explicitly unpoisoning the redzone instead. That was my first attempt, but it resulted in test failures due to the above. > ... > > > +__no_sanitize_memory > > +static inline void *memset_no_sanitize_memory(void *s, int c, > > size_t n) > > +{ > > + return memset(s, c, n); > > +} > > I think depending on the compiler optimizations this might end up > being a call to normal memset, that would still change the shadow > bytes. Interesting, do you have some specific scenario in mind? I vaguely remember that in the past there were cases when sanitizer annotations were lost after inlining, but I thought they were sorted out? And, in any case, if this were to happen, would not it be considered a compiler bug that needs fixing there, and not in the kernel?
> A problem with __memset() is that, at least for me, it always ends > up being a call. There is a use case where we need to write only 1 > byte, so I thought that introducing a call there (when compiling > without KMSAN) would be unacceptable. Wonder what happens with that use case if we e.g. build with fortify-source. Calling memset() for a single byte might be indicating the code is not hot. > > ... > > > > > +__no_sanitize_memory > > > +static inline void *memset_no_sanitize_memory(void *s, int c, > > > size_t n) > > > +{ > > > + return memset(s, c, n); > > > +} > > > > I think depending on the compiler optimizations this might end up > > being a call to normal memset, that would still change the shadow > > bytes. > > Interesting, do you have some specific scenario in mind? I vaguely > remember that in the past there were cases when sanitizer annotations > were lost after inlining, but I thought they were sorted out? Sanitizer annotations are indeed lost after inlining, and we cannot do much about that. They are implemented using function attributes, and if a function dissolves after inlining, we cannot possibly know which instructions belonged to it. Consider the following example (also available at https://godbolt.org/z/5r7817G8e): ================================== void *kmalloc(int size); __attribute__((no_sanitize("kernel-memory"))) __attribute__((always_inline)) static void *memset_nosanitize(void *s, int c, int n) { return __builtin_memset(s, c, n); } void *do_something_nosanitize(int size) { void *ptr = kmalloc(size); memset_nosanitize(ptr, 0, size); return ptr; } void *do_something_sanitize(int size) { void *ptr = kmalloc(size); __builtin_memset(ptr, 0, size); return ptr; } ================================== If memset_nosanitize() has __attribute__((always_inline)), the compiler generates the same LLVM IR calling __msan_memset() for both do_something_nosanitize() and do_something_sanitize(). If we comment out this attribute, do_something_nosanitize() calls memset_nosanitize(), which doesn't have the sanitize_memory attribute. But even now __builtin_memset() is still calling __msan_memset(), because __attribute__((no_sanitize("kernel-memory"))) somewhat counterintuitively still preserves some instrumentation (see include/linux/compiler-clang.h for details). Replacing __attribute__((no_sanitize("kernel-memory"))) with __attribute__((disable_sanitizer_instrumentation)) fixes this situation: define internal fastcc noundef ptr @memset_nosanitize(void*, int, int)(ptr noundef returned writeonly %s, i32 noundef %n) unnamed_addr #2 { entry: %conv = sext i32 %n to i64 tail call void @llvm.memset.p0.i64(ptr align 1 %s, i8 0, i64 %conv, i1 false) ret ptr %s } > > And, in any case, if this were to happen, would not it be considered a > compiler bug that needs fixing there, and not in the kernel? As stated above, I don't think this is more or less working as intended. If we really want the ability to inline __memset(), we could transform it into memset() in non-sanitizer builds, but perhaps having a call is also acceptable?
On Fri, 2023-12-08 at 16:25 +0100, Alexander Potapenko wrote: > > A problem with __memset() is that, at least for me, it always ends > > up being a call. There is a use case where we need to write only 1 > > byte, so I thought that introducing a call there (when compiling > > without KMSAN) would be unacceptable. > > Wonder what happens with that use case if we e.g. build with fortify- > source. > Calling memset() for a single byte might be indicating the code is > not hot. The original code has a simple assignment. Here is the relevant diff: if (s->flags & __OBJECT_POISON) { - memset(p, POISON_FREE, poison_size - 1); - p[poison_size - 1] = POISON_END; + memset_no_sanitize_memory(p, POISON_FREE, poison_size - 1); + memset_no_sanitize_memory(p + poison_size - 1, POISON_END, 1); } [...] > As stated above, I don't think this is more or less working as > intended. > If we really want the ability to inline __memset(), we could > transform > it into memset() in non-sanitizer builds, but perhaps having a call > is > also acceptable? Thanks for the detailed explanation and analysis. I will post a version with a __memset() and let the slab maintainers decide if the additional overhead is acceptable.
On Wed, 2023-12-13 at 02:31 +0100, Ilya Leoshkevich wrote: > On Fri, 2023-12-08 at 16:25 +0100, Alexander Potapenko wrote: > > > A problem with __memset() is that, at least for me, it always > > > ends > > > up being a call. There is a use case where we need to write only > > > 1 > > > byte, so I thought that introducing a call there (when compiling > > > without KMSAN) would be unacceptable. [...] > > As stated above, I don't think this is more or less working as > > intended. > > If we really want the ability to inline __memset(), we could > > transform > > it into memset() in non-sanitizer builds, but perhaps having a call > > is > > also acceptable? > > Thanks for the detailed explanation and analysis. I will post > a version with a __memset() and let the slab maintainers decide if > the additional overhead is acceptable. I noticed I had the same problem in the get_user()/put_user() and check_canary() patches. The annotation being silently ignored is never what a programmer intends, so what do you think about adding noinline to __no_kmsan_checks and __no_sanitize_memory?
diff --git a/include/linux/kmsan.h b/include/linux/kmsan.h index ff8fd95733fa..439df72c8dc6 100644 --- a/include/linux/kmsan.h +++ b/include/linux/kmsan.h @@ -345,4 +345,13 @@ static inline void *kmsan_get_metadata(void *addr, bool is_origin) #endif +/** + * memset_no_sanitize_memory() - memset() without the KMSAN instrumentation. + */ +__no_sanitize_memory +static inline void *memset_no_sanitize_memory(void *s, int c, size_t n) +{ + return memset(s, c, n); +} + #endif /* _LINUX_KMSAN_H */
Add a wrapper for memset() that prevents unpoisoning. This is useful for filling memory allocator redzones. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- include/linux/kmsan.h | 9 +++++++++ 1 file changed, 9 insertions(+)