2018-07-16 10:02:12

by Ursula Braun

Subject: [PATCH net 1/4] net/smc: take sock lock in smc_ioctl()

From: Ursula Braun <[email protected]>

SMC ioctl processing requires the sock lock to work properly in
all thinkable scenarios.
Problem has been found with RaceFuzzer and fixes:
KASAN: null-ptr-deref Read in smc_ioctl

Reported-by: Byoungyoung Lee <[email protected]>
Reported-by: [email protected]
Signed-off-by: Ursula Braun <[email protected]>
---
net/smc/af_smc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 5334157f5065..a4381b38a521 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1524,6 +1524,7 @@ static int smc_ioctl(struct socket *sock, unsigned int cmd,
return -EBADF;
return smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
}
+ lock_sock(&smc->sk);
switch (cmd) {
case SIOCINQ: /* same as FIONREAD */
if (smc->sk.sk_state == SMC_LISTEN)
@@ -1573,8 +1574,10 @@ static int smc_ioctl(struct socket *sock, unsigned int cmd,
}
break;
default:
+ release_sock(&smc->sk);
return -ENOIOCTLCMD;
}
+ release_sock(&smc->sk);

return put_user(answ, (int __user *)arg);
}
--
2.16.4

2018-07-16 10:10:56

by Stefano Brivio

Subject: Re: [PATCH net 1/4] net/smc: take sock lock in smc_ioctl()

On Mon, 16 Jul 2018 12:01:01 +0200
Ursula Braun <[email protected]> wrote:

> From: Ursula Braun <[email protected]>
>
> SMC ioctl processing requires the sock lock to work properly in
> all thinkable scenarios.
> Problem has been found with RaceFuzzer and fixes:
> KASAN: null-ptr-deref Read in smc_ioctl
>
> Reported-by: Byoungyoung Lee <[email protected]>
> Reported-by: [email protected]
> Signed-off-by: Ursula Braun <[email protected]>
> ---
> net/smc/af_smc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 5334157f5065..a4381b38a521 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1524,6 +1524,7 @@ static int smc_ioctl(struct socket *sock, unsigned int cmd,
> return -EBADF;
> return smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
> }
> + lock_sock(&smc->sk);
> switch (cmd) {
> case SIOCINQ: /* same as FIONREAD */
> if (smc->sk.sk_state == SMC_LISTEN)

return -EINVAL;

you should also unlock here, and:

case SIOCOUTQ:
/* output queue size (not send + not acked) */
if (smc->sk.sk_state == SMC_LISTEN)
return -EINVAL;

here, and:

case SIOCOUTQNSD:
/* output queue size (not send only) */
if (smc->sk.sk_state == SMC_LISTEN)
return -EINVAL;

here, and:

case SIOCATMARK:
if (smc->sk.sk_state == SMC_LISTEN)
return -EINVAL;

here.

--
Stefano

2018-07-16 11:57:49

by Ursula Braun

Subject: Re: [PATCH net 1/4] net/smc: take sock lock in smc_ioctl()

On 07/16/2018 12:09 PM, Stefano Brivio wrote:
> On Mon, 16 Jul 2018 12:01:01 +0200
> Ursula Braun <[email protected]> wrote:
>
>> From: Ursula Braun <[email protected]>
>>
>> SMC ioctl processing requires the sock lock to work properly in
>> all thinkable scenarios.
>> Problem has been found with RaceFuzzer and fixes:
>> KASAN: null-ptr-deref Read in smc_ioctl
>>
>> Reported-by: Byoungyoung Lee <[email protected]>
>> Reported-by: [email protected]
>> Signed-off-by: Ursula Braun <[email protected]>
>> ---
>> net/smc/af_smc.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 5334157f5065..a4381b38a521 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -1524,6 +1524,7 @@ static int smc_ioctl(struct socket *sock, unsigned int cmd,
>> return -EBADF;
>> return smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
>> }
>> + lock_sock(&smc->sk);
>> switch (cmd) {
>> case SIOCINQ: /* same as FIONREAD */
>> if (smc->sk.sk_state == SMC_LISTEN)
>
> return -EINVAL;
>
> you should also unlock here, and:
>
> case SIOCOUTQ:
> /* output queue size (not send + not acked) */
> if (smc->sk.sk_state == SMC_LISTEN)
> return -EINVAL;
>
> here, and:
>
> case SIOCOUTQNSD:
> /* output queue size (not send only) */
> if (smc->sk.sk_state == SMC_LISTEN)
> return -EINVAL;
>
> here, and:
>
> case SIOCATMARK:
> if (smc->sk.sk_state == SMC_LISTEN)
> return -EINVAL;
>
> here.
>

sorry, my fault! V2 is on its way. Thanks for your hint.