On 11/21/19 10:58 PM, Dmitry Vyukov wrote:

On Thu, Nov 21, 2019 at 1:27 PM Andrey Ryabinin [off-list ref] wrote:

quoted

quoted

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 6814d6d6a023..4bfce0af881f 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c

@@ -102,7 +102,8 @@ EXPORT_SYMBOL(__kasan_check_write);
 #undef memset
 void *memset(void *addr, int c, size_t len)
 {
-     check_memory_region((unsigned long)addr, len, true, _RET_IP_);
+     if (!check_memory_region((unsigned long)addr, len, true, _RET_IP_))
+             return NULL;

      return __memset(addr, c, len);
 }

@@ -110,8 +111,9 @@ void *memset(void *addr, int c, size_t len)
 #undef memmove
 void *memmove(void *dest, const void *src, size_t len)
 {
-     check_memory_region((unsigned long)src, len, false, _RET_IP_);
-     check_memory_region((unsigned long)dest, len, true, _RET_IP_);
+     if (!check_memory_region((unsigned long)src, len, false, _RET_IP_) ||
+         !check_memory_region((unsigned long)dest, len, true, _RET_IP_))
+             return NULL;

      return __memmove(dest, src, len);
 }

@@ -119,8 +121,9 @@ void *memmove(void *dest, const void *src, size_t len)
 #undef memcpy
 void *memcpy(void *dest, const void *src, size_t len)
 {
-     check_memory_region((unsigned long)src, len, false, _RET_IP_);
-     check_memory_region((unsigned long)dest, len, true, _RET_IP_);
+     if (!check_memory_region((unsigned long)src, len, false, _RET_IP_) ||
+         !check_memory_region((unsigned long)dest, len, true, _RET_IP_))
+             return NULL;

I realized that we are going a wrong direction here. Entirely skipping mem*() operation on any
poisoned shadow value might only make things worse. Some bugs just don't have any serious consequences,
but skipping the mem*() ops entirely might introduce such consequences, which wouldn't happen otherwise.

So let's keep this code as this, no need to check the result of check_memory_region().

I suggested it.

For our production runs it won't matter, we always panic on first report.
If one does not panic, there is no right answer. You say: _some_ bugs
don't have any serious consequences, but skipping the mem*() ops
entirely might introduce such consequences. The opposite is true as
well, right? :) And it's not hard to come up with a scenario where
overwriting memory after free or out of bounds badly corrupts memory.
I don't think we can somehow magically avoid bad consequences in all
cases.

Absolutely right. My point was that if it's bad consequences either way,
than there is no point in complicating this code, it doesn't buy us anything.

 

What I was thinking about is tests. We need tests for this. And we
tried to construct tests specifically so that they don't badly corrupt
memory (e.g. OOB/UAF reads, or writes to unused redzones, etc), so
that it's possible to run all of them to completion reliably. Skipping
the actual memory options allows to write such tests for all possible
scenarios. That's was my motivation.

But I see you point now. No objections to the patch in that case.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel