linux-kernel - Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses for 32-bit traces

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: <55FC1FC4.8070807@arm.com>
Date:	Fri, 18 Sep 2015 15:29:24 +0100
From:	Kapileshwar Singh <kapileshwar.singh@....com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	Namhyung Kim <namhyung@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Javi Merino <Javi.Merino@....com>,
	David Ahern <dsahern@...il.com>, Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH] tools lib traceevent: Mask higher bits of str addresses
 for 32-bit traces

Hi Steve,

On 18/09/15 14:45, Steven Rostedt wrote:
> On Fri, 18 Sep 2015 11:55:47 +0100
> Kapileshwar Singh <kapileshwar.singh@....com> wrote:
> 
>>>>> Perhaps we need to make addr into a unsigned long long, and then add:
>>>>>
>>>>>       addr = (pevent->long_size == 8) ?
>>>>>               *(unsigned long long *)(data + field->offset) :
>>>>>               (unsigned long long )*(unsigned int *)(data + field->offset);
>>>
>>> What about this? (untested)
>>>
>>>     addr = *(uint64_t *)(data + field->offset) &
>>>                         ((1ULL << pevent->long_size * 8) - 1);
>>
>> I tested this and it works fine.
> 
> Except that I think it may be buggy.
> 
>>
>>>
>>> Do we also need to consider byte endians?  Maybe it'd be better adding
>>> a helper to dereference pointers then..
> 
> Yes and no.
> 
>>
>> In this particular case, since the address is just a key for a lookup into the 
>> printk_map, which seems like a (addr -> const char *) mapping for string 
>> literals in the trace file, the endian-ness should not matter (I could be wrong though).
> 
> Correct, which is why I said "no", BUT! this is why I think Namhyung's
> version may be buggy (besides the overflow of the buffer).
> 
> If this is a 64 bit big endian reading a 32 bit little endian file, I
> think the result will be incorrect.
> 
> The *(uint64_t *) will return a 64bit number, but the address (with
> long_size == 4) only needs 32bits. Thus, we are getting 32 more bits
> than needed. Let's say the address is 0x12345678 that is loaded in the
> file. Being little endian, it would be loaded as "78 56 34 12". Let's
> say the 32bits after that is 0xDEADBEEF, loaded as "EF BE AD DE". Now
> the number returned to addr (being a 64 bit big endian) would be:
> 0x785643412EFBEADDE  But then we do the shift:
> 
>  (1ULL << pevent->long_size * 8) - 1; which would leave us with:
> 
> 0xEFBEADDE
> 
> Not what we wanted.

Agreed.

> 
> My version only reads the necessary bytes, and also wont suffer from
> reading past the data size of the buffer (which is another bug).
> 

Thanks for noticing and explaining this, makes perfect sense now!

Will submit a v3 for this.

Regards, 
KP

> -- Steve
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/