linux-kernel - Re: [PATCH] fs/ncpfs: fix error paths and goto statements in ncp_fill

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]

Date:	Wed, 14 Dec 2011 06:52:45 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Djalal Harouni <tixxdz@...ndz.org>
Cc:	David Miller <davem@...emloft.net>,
	Nick Piggin <npiggin@...nel.dk>,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs/ncpfs: fix error paths and goto statements in
 ncp_fill_super()

[davem Cc'd, since there's an ugly socket-related problem in that area]

On Tue, Dec 13, 2011 at 02:47:29AM +0100, Djalal Harouni wrote:
> The label 'out_bdi' should be followed by bdi_destroy() instead of
> fput() which should be after the 'out_fput' label.
> 
> If bdi_setup_and_register() fails then jump to the 'out_fput' label
> instead of the 'out_bdi' one.
> 
> If fget(data.info_fd) fails then jump to the previously fixed 'out_bdi'
> label to call bdi_destroy() otherwise the bdi object will not be
> destroyed.
> 
> Compile tested only.

Applied, but now that I've looked at that code...  It, along with several
other places, overwrites a bunch of fields in sock->sk.  What happens if
some joker feeds that descriptor to, say, svc_addsock()?  It does the
same kind of thing, and both assume that no bonehead programmer would
ever do something that dumb.  Moreover, what if somebody feeds the same fd
to ncpfs mount or svc_addsock twice?

I'd done a quick grep and AFAICS, most of the places doing that kind of thing
is acting on internally created socket, so no such problem for them.  Or
they take care to check that somebody has already done that to socket in
question.  Potentially unpleasant ones:
	drivers/scsi/iscsi_tcp.c:iscsi_sw_tcp_conn_bind()
	fs/dlm/lowcomms.c:process_sctp_notification()
	fs/ncpfs/inode.c:ncp_fill_super()
	net/sunrpc/svcsock.c:svc_addsock()

I'm not familiar with that area at all; not even enough to tell if locking
is wrong in svc_addsock() - I suspect that it is, but...

Dave, am I right assuming that all those places ought to check if somebody
is already parasiting on the socket in question and fail with -EBUSY or
something like that?
--
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/