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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 18 Sep 2015 09:45:52 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Kapileshwar Singh <kapileshwar.singh@....com>
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

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.

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

-- 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/