netdev - Re: "ss -p" segfaults

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: <1587116.GuyXMaAcLE@fb07-iapwap2>
Date:	Thu, 16 Jul 2015 08:37:49 +0200
From:	Marc Dietrich <marvin24@....de>
To:	Vadim Kochan <vadim4j@...il.com>
Cc:	"Rustad, Mark D" <mark.d.rustad@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: "ss -p" segfaults

Am Donnerstag, 16. Juli 2015, 01:22:38 schrieb Vadim Kochan:
> On Wed, Jul 15, 2015 at 09:57:51PM +0300, Vadim Kochan wrote:
> > On Wed, Jul 15, 2015 at 06:52:49PM +0000, Rustad, Mark D wrote:
> > > > On Jul 15, 2015, at 9:49 AM, Rustad, Mark D <mark.d.rustad@...el.com> 
wrote:
> > > >> On Jul 15, 2015, at 8:12 AM, Vadim Kochan <vadim4j@...il.com> wrote:
> > > >> Would you please check this fix ?
> > > >> 
> > > >> diff --git a/misc/ss.c b/misc/ss.c
> > > >> index 03f92fa..3a826e4 100644
> > > >> --- a/misc/ss.c
> > > >> +++ b/misc/ss.c
> > > >> @@ -683,8 +683,8 @@ static inline void sock_addr_set_str(inet_prefix
> > > >> *prefix, char **ptr)
> > > >> 
> > > >> static inline char *sock_addr_get_str(const inet_prefix *prefix)
> > > >> {
> > > >> -    char *tmp ;
> > > >> -    memcpy(&tmp, prefix->data, sizeof(char *));
> > > >> +    char *tmp;
> > > >> +    memcpy(&tmp, &prefix->data[0], sizeof(char *));
> > > >> 
> > > >>    return tmp;
> > > >> 
> > > >> }
> > > > 
> > > > That surely is not a fix! The destination of the memcpy is the address
> > > > of an uninitialized stack variable! Both versions are equally bad.> > 
> > > I probably over-reacted, but using memcpy to access a pointer in this
> > > way is just ugly. For one thing, it circumvents any sanity-checking
> > > that the compiler can do. And changing the prefix->data to
> > > &prefix->data[0] should be exactly the same thing and therefore should
> > > not fix anything. Anyway, never mind that.
> > > 
> > > Looking at more of the code, it looks to me like the the string pointer
> > > in data can sometimes point to a literal string instead of allocated
> > > memory when proc is in use. Free would not be happy with that. Look at
> > > the use of variable peer in function unix_stats_print.> 
> > Yes that right, I am already looking on this ...
> > 
> > > --
> > > Mark Rustad, Networking Division, Intel Corporation
> 
> I did partially revert of the buggy commit and it does not crash, but I will
> do more testing, and after will send the patch and will try to prepare some
> test scripts for ss.
> 
> The crash appears only if to dump processes info from /proc, which might
> be caused that netlink stats returned error, probably by wrong request
> (not supported attribute or flag ?).

the reason it uses proc for me is my self compiled (and trimmed) kernel which 
disabled all *_diag modules which seem to be required by ss. I didn't know 
this. On the other hand, I managed to find a bug this way :-)

Marc


Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)