Skip to contentSkip to conversation

kasan: fix last shadow judgement in memory_is_poisoned_16()

PATCH Superseded Xishi Qiu 2 participants 5 messages Sep 8, 2015

Subsystems (2)
Memory management KASAN

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Xishi Qiu

author
#0 lore raw

PATCH 1/1 kasan: fix last shadow judgement in memory_is_poisoned_16()

The shadow which correspond 16 bytes may span 2 or 3 bytes. If shadow only take 2 bytes, we can return in "if (likely(!last_byte)) ...", but it calculates wrong, so fix it.

Signed-off-by Xishi Qiu <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +1 -2
index 7b28e9c..8da2114 100644
@@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
 
 	if (unlikely(*shadow_addr)) {
 		u16 shadow_first_bytes = *(u16 *)shadow_addr;
-		s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
 
 		if (unlikely(shadow_first_bytes))
 			return true;
 
-		if (likely(!last_byte))
+		if (likely(IS_ALIGNED(addr, 8)))
 			return false;
 
 		return memory_is_poisoned_1(addr + 15);
Signature · 1 line
1.7.1
maintainer
#1 lore raw

2015-09-08 4:42 GMT+03:00 Xishi Qiu <[email protected]>:

Quoted 4 lines

The shadow which correspond 16 bytes may span 2 or 3 bytes. If shadow only take 2 bytes, we can return in "if (likely(!last_byte)) ...", but it calculates wrong, so fix it.

Please, be more specific. Describe what is wrong with the current code and why, what's the effect of this bug and how you fixed it.

Quoted 24 lines
Signed-off-by Xishi Qiu <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +1 -3
index 7b28e9c..8da2114 100644
@@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
 
        if (unlikely(*shadow_addr)) {
                u16 shadow_first_bytes = *(u16 *)shadow_addr;
-               s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
 
                if (unlikely(shadow_first_bytes))
                        return true;
 
-               if (likely(!last_byte))
+               if (likely(IS_ALIGNED(addr, 8)))
                        return false;
 
                return memory_is_poisoned_1(addr + 15);
--
Quoted 3 lines

1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#2 lore raw
Quoted 10 lines · Andrey Ryabinin

2015-09-08 4:42 GMT+03:00 Xishi Qiu <[email protected]>:

Quoted 4 lines

The shadow which correspond 16 bytes may span 2 or 3 bytes. If shadow only take 2 bytes, we can return in "if (likely(!last_byte)) ...", but it calculates wrong, so fix it.

Please, be more specific. Describe what is wrong with the current code and why, what's the effect of this bug and how you fixed it.

If the 16 bytes memory is aligned on 8, then the shadow takes only 2 bytes.
So we check "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);". The code "if (likely(IS_ALIGNED(addr, 8)))" is wrong judgement.

e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will continue to call "return memory_is_poisoned_1(addr + 15);"

Thanks,
Xishi Qiu

Quoted 30 lines
Quoted 24 lines
Signed-off-by Xishi Qiu <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +1 -3
index 7b28e9c..8da2114 100644
@@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
 
        if (unlikely(*shadow_addr)) {
                u16 shadow_first_bytes = *(u16 *)shadow_addr;
-               s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
 
                if (unlikely(shadow_first_bytes))
                        return true;
 
-               if (likely(!last_byte))
+               if (likely(IS_ALIGNED(addr, 8)))
                        return false;
 
                return memory_is_poisoned_1(addr + 15);
--
Quoted 3 lines

1.7.1

.

author
#3 lore raw
Quoted 8 lines · Xishi Qiu
Quoted 6 lines · Andrey Ryabinin

2015-09-08 4:42 GMT+03:00 Xishi Qiu <[email protected]>:

Quoted 4 lines

The shadow which correspond 16 bytes may span 2 or 3 bytes. If shadow only take 2 bytes, we can return in "if (likely(!last_byte)) ...", but it calculates wrong, so fix it.

Quoted 8 lines
Quoted 4 lines

Please, be more specific. Describe what is wrong with the current code and why, what's the effect of this bug and how you fixed it.

If the 16 bytes memory is aligned on 8, then the shadow takes only 2 bytes.
So we check "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);". The code "if (likely(IS_ALIGNED(addr, 8)))" is wrong judgement.

Sorry, a mistake, The code "if (likely(!last_byte))" is wrong judgement.

Quoted 38 lines

e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will continue to call "return memory_is_poisoned_1(addr + 15);"

Thanks,
Xishi Qiu

Quoted 30 lines
Quoted 24 lines
Signed-off-by Xishi Qiu <[email protected]>
---
 mm/kasan/kasan.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
mm/kasan/kasan.c +1 -3
index 7b28e9c..8da2114 100644
@@ -135,12 +135,11 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
 
        if (unlikely(*shadow_addr)) {
                u16 shadow_first_bytes = *(u16 *)shadow_addr;
-               s8 last_byte = (addr + 15) & KASAN_SHADOW_MASK;
 
                if (unlikely(shadow_first_bytes))
                        return true;
 
-               if (likely(!last_byte))
+               if (likely(IS_ALIGNED(addr, 8)))
                        return false;
 
                return memory_is_poisoned_1(addr + 15);
--
Quoted 3 lines

1.7.1

.

maintainer
#4 lore raw

2015-09-08 12:52 GMT+03:00 Xishi Qiu <[email protected]>:

Quoted 15 lines · Xishi Qiu
Quoted 14 lines · Andrey Ryabinin
Quoted 11 lines

2015-09-08 4:42 GMT+03:00 Xishi Qiu <[email protected]>:

Quoted 4 lines

The shadow which correspond 16 bytes may span 2 or 3 bytes. If shadow only take 2 bytes, we can return in "if (likely(!last_byte)) ...", but it calculates wrong, so fix it.

Please, be more specific. Describe what is wrong with the current code and why, what's the effect of this bug and how you fixed it.

Quoted 9 lines
Quoted 3 lines

If the 16 bytes memory is aligned on 8, then the shadow takes only 2 bytes.
So we check "shadow_first_bytes" is enough, and need not to call "memory_is_poisoned_1(addr + 15);". The code "if (likely(IS_ALIGNED(addr, 8)))" is wrong judgement.

Sorry, a mistake, The code "if (likely(!last_byte))" is wrong judgement.

Quoted 3 lines

e.g. addr=0, so last_byte = 15 & KASAN_SHADOW_MASK = 7, then the code will continue to call "return memory_is_poisoned_1(addr + 15);"

Right, put this into changelog please.

Quoted 3 lines
Quoted 3 lines

Thanks,
Xishi Qiu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>