linux-kernel - Re: [PATCH] perf trace: Fix unaligned access for augmented args

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z4A7NdLPeXmplTXA@x1>
Date: Thu, 9 Jan 2025 18:10:13 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org, bpf@...r.kernel.org,
	Howard Chu <howardchu95@...il.com>
Subject: Re: [PATCH] perf trace: Fix unaligned access for augmented args

On Thu, Jan 09, 2025 at 03:46:33PM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jan 02, 2025 at 12:12:47PM -0800, Namhyung Kim wrote:
> > Some version of compilers reported unaligned accesses in perf trace when
> > undefined-behavior sanitizer is on.  I found that it uses raw data in the
> > sample directly and assuming it's properly aligned.

> > Unlike other sample fields, the raw data is not 8-byte aligned because
> > there's a size field (u32) before the actual data.  So I added a static
> > buffer in syscall__augmented_args() and return it instead.  This is not
> > ideal but should work well as perf trace is single-threaded.
 
> > A better approach would be aligning the raw data by adding a 4-byte data
> > before the augmented args but I'm afraid it'd break the backward
> > compatibility.
  
> You mean for 'perf trace record' files?
 
> Older tools will not be able to process new files, while old files will
> be remain processable by new tools if we insert a u32 with zeroes before
> the size field, that way if the first u32 is not zero, we do as you do
> below and incur the cost of copying to that intermediary buffer,
> otherwise we read the real size in the next u32 and don't incur the cost
> of copying.
 
> Your fix below works as it incurs the cost all the time, which is ok for
> now, but as a follow up patch we can see if the approach I described
> above works.
 
> Applying.

Applied, and forget about my suggestion above, as we discussed this is
not feasible, I added these notes to your patch:

   Committer testing:
    
    To build with the undefined behaviour sanitizer:
    
     $ make CC=clang EXTRA_CFLAGS=-fsanitize=undefined -C tools/perf
    
    Checking if the resulting binary is instrumented:
    
      root@...ber:~# nm ~/bin/perf | grep ubsan | wc -l
      113
      root@...ber:~# nm ~/bin/perf | grep ubsan | tail -5
      000000000043d5b0 t _ZN7__ubsanL19UBsanOnDeadlySignalEiPvS0_
      000000000043ce50 T _ZNK7__ubsan5Value12getSIntValueEv
      000000000043cf40 T _ZNK7__ubsan5Value12getUIntValueEv
      000000000043d140 T _ZNK7__ubsan5Value13getFloatValueEv
      000000000043cfd0 T _ZNK7__ubsan5Value19getPositiveIntValueEv
      root@...ber:~#
    
    Now running something that will access timespec, as reported in the
    Closes URL:
    
      root@...ber:~# perf trace --max-events=1 -e *nano* sleep 1.1
      trace/beauty/timespec.c:10:64: runtime error: member access within misaligned address 0x7fc583cfb2a4 for type 'struct augmented_arg', which requires 8 byte alignment
      0x7fc583cfb2a4: note: pointer points here
        99 99 11 00 10 00 00 00  00 00 00 00 01 00 00 00  00 00 00 00 01 e1 f5 05  00 00 00 00 00 00 00 00
                    ^
      SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior trace/beauty/timespec.c:10:64
      <SNIP>
    
    As Namhyung said we need to make the raw_data to be 64-bit aligned,
    probably we need to add a PERF_SAMPLE_ALIGNED_RAW with a 64-bit raw_size
    instead of the current u32 done at kernel/events/core.c,
    perf_output_sample(), that perf_output_put(handle, raw->size) where
    raw->size is an u32 and then the raw_data is always 64-bit unaligned...
    
    After the patch:
    
      root@...ber:~# perf trace -e *nano* sleep 1.1
           0.000 (1100.064 ms): sleep/1984224 clock_nanosleep(rqtp: { .tv_sec: 1, .tv_nsec: 100000001 }, rmtp: 0x7fff5b3fe970) = 0
      root@...ber:~#