On Tue, 26 Apr 2022, Jakub Jelinek wrote:
> Hi!
>
> On the following testcase, we have in main's frame 3 variables,
> some red zone padding, 4 byte d, followed by 12 bytes of red zone padding,
> then
> 8 byte b followed by 24 bytes of red zone padding, then 40 bytes c followed
> by some red zone padding.
> The intended content of shadow memory for that is (note, each byte describes
> 8 bytes of memory):
> f1 f1 f1 f1 04 f2 00 f2 f2 f2 00 00 00 00 00 f3 f3 f3 f3 f3
> left red d mr b middle r c right red zone
>
> f1 is left red zone magic
> f2 is middle red zone magic
> f3 is right red zone magic
> 00 when all 8 bytes are accessible
> 01-07 when only 1 to 7 bytes are accessible followed by inaccessible bytes
>
> The -fdump-rtl-expand-details dump makes it clear that it misbehaves:
> Flushing rzbuffer at offset -160 with: f1 f1 f1 f1
> Flushing rzbuffer at offset -128 with: 04 f2 00 00
> Flushing rzbuffer at offset -128 with: 00 00 00 f2
> Flushing rzbuffer at offset -96 with: f2 f2 00 00
> Flushing rzbuffer at offset -64 with: 00 00 00 f3
> Flushing rzbuffer at offset -32 with: f3 f3 f3 f3
> In the end we end up with
> f1 f1 f1 f1 00 00 00 f2 f2 f2 00 00 00 00 00 f3 f3 f3 f3 f3
> shadow bytes because at offset -128 there are 2 overlapping stores
> as asan_redzone_buffer::emit_redzone_byte has flushed the temporary 4 byte
> buffer in the middle.
>
> The function is called with an offset and value. If the passed offset is
> consecutive with the prev_offset + buffer size (off == offset), then
> we handle it correctly, similarly if the new offset is far enough from the
> old one (we then flush whatever was in the buffer and if needed add up to 3
> bytes of 00 before actually pushing value.
>
> But what isn't handled correctly is when the offset isn't consecutive to
> what has been added last time, but it is in the same 4 byte word of shadow
> memory (32 bytes of actual memory), like the above case where
> we have consecutive 04 f2 and then skip one shadow memory byte (aka 8 bytes
> of real memory) and then want to emit f2. Emitting that as a store
> of little-endian 0x0000f204 followed by a store of 0xf2000000 to the same
> address doesn't work, we want to emit 0xf200f204.
>
> The following patch does that by pushing 1 or 2 00 bytes.
> Additionally, as a small cleanup, instead of using
> m_shadow_bytes.safe_push (value);
> flush_if_full ();
> in all of if, else if and else bodies it sinks those 2 stmts to the end
> of function as all do the same thing.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
> 2022-04-26 Jakub Jelinek <ja...@redhat.com>
>
> PR sanitizer/105396
> * asan.cc (asan_redzone_buffer::emit_redzone_byte): Handle the case
> where offset is bigger than off but smaller than m_prev_offset + 32
> bits by pushing one or more 0 bytes. Sink the
> m_shadow_bytes.safe_push (value); flush_if_full (); statements from
> all cases to the end of the function.
>
> * gcc.dg/asan/pr105396.c: New test.
>
> --- gcc/asan.cc.jj 2022-02-19 09:03:50.000000000 +0100
> +++ gcc/asan.cc 2022-04-26 16:57:49.737316329 +0200
> @@ -1497,10 +1497,14 @@ asan_redzone_buffer::emit_redzone_byte (
> HOST_WIDE_INT off
> = m_prev_offset + ASAN_SHADOW_GRANULARITY * m_shadow_bytes.length ();
> if (off == offset)
> + /* Consecutive shadow memory byte. */;
> + else if (offset < m_prev_offset + (HOST_WIDE_INT) (ASAN_SHADOW_GRANULARITY
> + * RZ_BUFFER_SIZE)
> + && !m_shadow_bytes.is_empty ())
> {
> - /* Consecutive shadow memory byte. */
> - m_shadow_bytes.safe_push (value);
> - flush_if_full ();
> + /* Shadow memory byte with a small gap. */
> + for (; off < offset; off += ASAN_SHADOW_GRANULARITY)
> + m_shadow_bytes.safe_push (0);
> }
> else
> {
> @@ -1521,9 +1525,9 @@ asan_redzone_buffer::emit_redzone_byte (
> m_shadow_mem = adjust_address (m_shadow_mem, VOIDmode,
> diff >> ASAN_SHADOW_SHIFT);
> m_prev_offset = offset;
> - m_shadow_bytes.safe_push (value);
> - flush_if_full ();
> }
> + m_shadow_bytes.safe_push (value);
> + flush_if_full ();
> }
>
> /* Emit RTX emission of the content of the buffer. */
> --- gcc/testsuite/gcc.dg/asan/pr105396.c.jj 2022-04-26 16:56:34.522348879
> +0200
> +++ gcc/testsuite/gcc.dg/asan/pr105396.c 2022-04-26 17:00:54.757776387
> +0200
> @@ -0,0 +1,19 @@
> +/* PR sanitizer/105396 */
> +/* { dg-do run } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +/* { dg-shouldfail "asan" } */
> +
> +int
> +main ()
> +{
> + int a;
> + int *b[1];
> + int c[10];
> + int d[1][1];
> + for (a = 0; a < 1; a++)
> + d[1][a] = 0;
> + return 0;
> +}
> +
> +/* { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on
> address.*(\n|\r\n|\r)" } */
> +/* { dg-output "WRITE of size.*" } */
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
