netdev - RE: [PATCH] Phonet: set the pipe handle using setsockopt

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: <81C3A93C17462B4BBD7E272753C105791FB090B0B8@EXDCVYMBSTM005.EQ1STM.local>
Date:	Mon, 14 Nov 2011 11:36:12 +0100
From:	Hemant-vilas RAMDASI <hemant.ramdasi@...ricsson.com>
To:	Rémi Denis-Courmont 
	<remi.denis-courmont@...ia.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"netdev-owner@...r.kernel.org" <netdev-owner@...r.kernel.org>
Subject: RE: [PATCH] Phonet: set the pipe handle using setsockopt

Remi,

> -----Original Message-----
> From: Rémi Denis-Courmont [mailto:remi.denis-courmont@...ia.com]
> Sent: Monday, November 14, 2011 3:00 PM
> To: Hemant-vilas RAMDASI; netdev@...r.kernel.org
> Subject: Re: [PATCH] Phonet: set the pipe handle using setsockopt
> > @@ -180,6 +182,7 @@ static inline __u8 pn_sockaddr_get_resource(const
> struct
> > sockaddr_pn *spn) /* Phonet device ioctl requests */
> >  #ifdef __KERNEL__
> >  #define SIOCPNGAUTOCONF		(SIOCDEVPRIVATE + 0)
> > +#define SIOPNPIPE_ENABLE	_IO(SIOCPNGAUTOCONF,   1)
> 
> Does this even work? I am not an expert on this, but I would think that
> device-private controls are routed to the network device, not the
> socket. In
> any case, it does not seem right.
> 
Yes, it works. The ioctl is routed to per-socket functions.

> > @@ -863,14 +898,31 @@ static int pep_sock_connect(struct sock *sk,
> struct
> > sockaddr *addr, int len) int err;
> >  	u8 data[4] = { 0 /* sub-blocks */, PAD, PAD, PAD };
> >
> > -	pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
> > +	if (pn->pipe_handle == PN_PIPE_INVALID_HANDLE)
> > +		pn->pipe_handle = 1; /* anything but INVALID_HANDLE */
> > +
> >  	err = pipe_handler_request(sk, PNS_PEP_CONNECT_REQ,
> > -					PN_PIPE_ENABLE, data, 4);
> > -	if (err) {
> > -		pn->pipe_handle = PN_PIPE_INVALID_HANDLE;
> 
> The current backlog functions assume that pipe_handle =
> PN_PIPE_INVALID_HANDLE
> if the socket is not yet connected. That's why the old code would clear
> the
> pipe_handle always on error.
Ok..I think clearing pipe-handle on connect error should take care of this.
User-space need to set the handle again.
> 
> So it is not that simple.
> 
> > @@ -894,6 +946,16 @@ static int pep_ioctl(struct sock *sk, int cmd,
> unsigned
> > +	case SIOPNPIPE_ENABLE:
> > +		if (sk->sk_state == TCP_SYN_SENT)
> > +			return -EBUSY;
> > +		else if (sk->sk_state == TCP_ESTABLISHED)
> > +			return -EISCONN;
> > +		else
> > +			return pep_sock_enable(sk, NULL, 0);
> > +		break;
> >  	}
> 
> I strongly suspect insufficient locking here.
Ok..
> 
> >
> >  	return -ENOIOCTLCMD;
> > @@ -959,6 +1021,18 @@ static int pep_setsockopt(struct sock *sk, int
> level,
> > int optname, }
> >  		goto out_norel;
> >
> > +	case PNPIPE_HANDLE:
> > +		if ((sk->sk_state == TCP_CLOSE) &&
> > +			(val >= 0) && (val < PN_PIPE_INVALID_HANDLE))
> > +			pn->pipe_handle = val;
> > +		else
> > +			err = -EINVAL;
> > +		break;
> 
> Same problem regarding pipe_handle as above.
I think locking already exists..

> > @@ -994,6 +1068,17 @@ static int pep_getsockopt(struct sock *sk, int
> level,
> > int optname, return -EINVAL;
> >  		break;
> >
> > +	case PNPIPE_ENABLE:
> > +		if (sk->sk_state == TCP_ESTABLISHED)
> > +			val = 1;
> > +		else
> > +			val = 0;
> > +		break;
> 
> Do you still need this read-only option?
Yes.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html