linux-kernel - Re: [PATCH v1] perf intel-pt: pkt-decoder: Fix alignment issues

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <93afcf44-5f8b-49c1-abc5-5304cba5f991@intel.com>
Date:   Thu, 5 Oct 2023 22:04:37 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Jiri Olsa <jolsa@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
        Liam Howlett <liam.howlett@...cle.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v1] perf intel-pt: pkt-decoder: Fix alignment issues

On 5/10/23 18:48, Ian Rogers wrote:
> On Mon, Oct 2, 2023 at 3:19 PM Ian Rogers <irogers@...gle.com> wrote:
>>
>> The byte aligned buffer is cast to large types and dereferenced
>> causing misaligned pointer warnings from undefined behavior sanitizer.
>> Fix the alignment issues with memcpy which may require the
>> introduction of temporaries.
>>
>> Signed-off-by: Ian Rogers <irogers@...gle.com>
>> ---
> 
> This is a relatively small change that fixes building with
> -fsanitize=alignment -fsanitize-undefined-trap-on-error. Adrian, as
> this is Intel-PT could you take a look?

Thanks! This has been down my list of things to do for ages,
but using get_unaligned_le16() etc seems nicer.  I sent a patch
set for that.

> 
> Thanks,
> Ian
> 
>>  .../intel-pt-decoder/intel-pt-pkt-decoder.c   | 21 ++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
>> index af9710622a1f..28659874d84e 100644
>> --- a/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
>> +++ b/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
>> @@ -83,7 +83,7 @@ static int intel_pt_get_long_tnt(const unsigned char *buf, size_t len,
>>         if (len < 8)
>>                 return INTEL_PT_NEED_MORE_BYTES;
>>
>> -       payload = le64_to_cpu(*(uint64_t *)buf);
>> +       memcpy_le64(&payload, buf, sizeof(payload));
>>
>>         for (count = 47; count; count--) {
>>                 if (payload & BIT63)
>> @@ -220,6 +220,8 @@ static int intel_pt_get_3byte(const unsigned char *buf, size_t len,
>>  static int intel_pt_get_ptwrite(const unsigned char *buf, size_t len,
>>                                 struct intel_pt_pkt *packet)
>>  {
>> +       uint32_t tmp;
>> +
>>         packet->count = (buf[1] >> 5) & 0x3;
>>         packet->type = buf[1] & BIT(7) ? INTEL_PT_PTWRITE_IP :
>>                                          INTEL_PT_PTWRITE;
>> @@ -228,12 +230,13 @@ static int intel_pt_get_ptwrite(const unsigned char *buf, size_t len,
>>         case 0:
>>                 if (len < 6)
>>                         return INTEL_PT_NEED_MORE_BYTES;
>> -               packet->payload = le32_to_cpu(*(uint32_t *)(buf + 2));
>> +               memcpy(&tmp, buf + 2, sizeof(tmp));
>> +               packet->payload = le32_to_cpu(tmp);
>>                 return 6;
>>         case 1:
>>                 if (len < 10)
>>                         return INTEL_PT_NEED_MORE_BYTES;
>> -               packet->payload = le64_to_cpu(*(uint64_t *)(buf + 2));
>> +               memcpy_le64(&packet->payload, buf + 2, sizeof(packet->payload));
>>                 return 10;
>>         default:
>>                 return INTEL_PT_BAD_PACKET;
>> @@ -258,7 +261,7 @@ static int intel_pt_get_mwait(const unsigned char *buf, size_t len,
>>         if (len < 10)
>>                 return INTEL_PT_NEED_MORE_BYTES;
>>         packet->type = INTEL_PT_MWAIT;
>> -       packet->payload = le64_to_cpu(*(uint64_t *)(buf + 2));
>> +       memcpy_le64(&packet->payload, buf + 2, sizeof(packet->payload));
>>         return 10;
>>  }
>>
>> @@ -454,6 +457,8 @@ static int intel_pt_get_ip(enum intel_pt_pkt_type type, unsigned int byte,
>>                            struct intel_pt_pkt *packet)
>>  {
>>         int ip_len;
>> +       uint16_t tmp16;
>> +       uint32_t tmp32;
>>
>>         packet->count = byte >> 5;
>>
>> @@ -465,13 +470,15 @@ static int intel_pt_get_ip(enum intel_pt_pkt_type type, unsigned int byte,
>>                 if (len < 3)
>>                         return INTEL_PT_NEED_MORE_BYTES;
>>                 ip_len = 2;
>> -               packet->payload = le16_to_cpu(*(uint16_t *)(buf + 1));
>> +               memcpy(&tmp16, buf + 1, sizeof(tmp16));
>> +               packet->payload = le16_to_cpu(tmp16);
>>                 break;
>>         case 2:
>>                 if (len < 5)
>>                         return INTEL_PT_NEED_MORE_BYTES;
>>                 ip_len = 4;
>> -               packet->payload = le32_to_cpu(*(uint32_t *)(buf + 1));
>> +               memcpy(&tmp32, buf + 1, sizeof(tmp32));
>> +               packet->payload = le32_to_cpu(tmp32);
>>                 break;
>>         case 3:
>>         case 4:
>> @@ -484,7 +491,7 @@ static int intel_pt_get_ip(enum intel_pt_pkt_type type, unsigned int byte,
>>                 if (len < 9)
>>                         return INTEL_PT_NEED_MORE_BYTES;
>>                 ip_len = 8;
>> -               packet->payload = le64_to_cpu(*(uint64_t *)(buf + 1));
>> +               memcpy_le64(&packet->payload, buf + 1, sizeof(packet->payload));
>>                 break;
>>         default:
>>                 return INTEL_PT_BAD_PACKET;
>> --
>> 2.42.0.582.g8ccd20d70d-goog
>>