Re: [Patch net] nfc: check sock state in llcp_sock_getname()
On Sat, Jan 2, 2016 at 12:03 AM, Dmitry Vyukov wrote: > On Sat, Jan 2, 2016 at 1:34 AM, Cong Wang wrote: >> llcp_sock_getname() checks llcp_sock->dev to make sure >> llcp_sock is already connected or bound, however, we could >> be in the middle of llcp_sock_bind() where llcp_sock->dev >> is bound and llcp_sock->service_name_len is set, >> but llcp_sock->service_name is not, in this case we would >> lead to copy some bytes from a NULL pointer. >> >> We should just check if sk->sk_state is still closed since >> both connect() and bind() will update this state at the end. > > Hi Cong, > > This is still racy. If you want to play lock-free then you also need > proper memory barriers. Stores to sk_state need to be > smp_store_release, while the load needs to be smp_load_acquire. > Otherwise getname still can see partially initialized socket. > Right... Or just lock sock perhaps. I will update my patch. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch net] nfc: check sock state in llcp_sock_getname()
On Sat, Jan 2, 2016 at 1:34 AM, Cong Wang wrote: > llcp_sock_getname() checks llcp_sock->dev to make sure > llcp_sock is already connected or bound, however, we could > be in the middle of llcp_sock_bind() where llcp_sock->dev > is bound and llcp_sock->service_name_len is set, > but llcp_sock->service_name is not, in this case we would > lead to copy some bytes from a NULL pointer. > > We should just check if sk->sk_state is still closed since > both connect() and bind() will update this state at the end. Hi Cong, This is still racy. If you want to play lock-free then you also need proper memory barriers. Stores to sk_state need to be smp_store_release, while the load needs to be smp_load_acquire. Otherwise getname still can see partially initialized socket. > Reported-by: Dmitry Vyukov > Cc: Lauro Ramos Venancio > Cc: Aloisio Almeida Jr > Cc: Samuel Ortiz > Signed-off-by: Cong Wang > --- > net/nfc/llcp_sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c > index ecf0a01..5a91997 100644 > --- a/net/nfc/llcp_sock.c > +++ b/net/nfc/llcp_sock.c > @@ -500,7 +500,7 @@ static int llcp_sock_getname(struct socket *sock, struct > sockaddr *uaddr, > struct nfc_llcp_sock *llcp_sock = nfc_llcp_sock(sk); > DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, llcp_addr, uaddr); > > - if (llcp_sock == NULL || llcp_sock->dev == NULL) > + if (llcp_sock == NULL || sk->sk_state == LLCP_CLOSED) > return -EBADFD; > > pr_debug("%p %d %d %d\n", sk, llcp_sock->target_idx, > -- > 1.8.3.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch net] nfc: check sock state in llcp_sock_getname()
llcp_sock_getname() checks llcp_sock->dev to make sure llcp_sock is already connected or bound, however, we could be in the middle of llcp_sock_bind() where llcp_sock->dev is bound and llcp_sock->service_name_len is set, but llcp_sock->service_name is not, in this case we would lead to copy some bytes from a NULL pointer. We should just check if sk->sk_state is still closed since both connect() and bind() will update this state at the end. Reported-by: Dmitry Vyukov Cc: Lauro Ramos Venancio Cc: Aloisio Almeida Jr Cc: Samuel Ortiz Signed-off-by: Cong Wang --- net/nfc/llcp_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c index ecf0a01..5a91997 100644 --- a/net/nfc/llcp_sock.c +++ b/net/nfc/llcp_sock.c @@ -500,7 +500,7 @@ static int llcp_sock_getname(struct socket *sock, struct sockaddr *uaddr, struct nfc_llcp_sock *llcp_sock = nfc_llcp_sock(sk); DECLARE_SOCKADDR(struct sockaddr_nfc_llcp *, llcp_addr, uaddr); - if (llcp_sock == NULL || llcp_sock->dev == NULL) + if (llcp_sock == NULL || sk->sk_state == LLCP_CLOSED) return -EBADFD; pr_debug("%p %d %d %d\n", sk, llcp_sock->target_idx, -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html