Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
From: 'Marcelo Ricardo Leitner' <marcelo.leitner@gmail.com>
Date: 2020-05-21 00:17:30
Also in:
linux-sctp
On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote: ...
Only SCTP_SOCKOPT_CONNECTX3 contains an indirect pointer. It is also the only getsockopt() that wants to return a buffer and an error code. It is also definitely abusing getsockopt().
...
quoted hunk ↗ jump to hunk
@@ -1375,11 +1350,11 @@ struct compat_sctp_getaddrs_old { #endif static int sctp_getsockopt_connectx3(struct sock *sk, int len, - char __user *optval, - int __user *optlen) + struct sctp_getaddrs_old *param, + int *optlen) { - struct sctp_getaddrs_old param; sctp_assoc_t assoc_id = 0; + struct sockaddr *addrs; int err = 0; #ifdef CONFIG_COMPAT@@ -1388,29 +1363,28 @@ static int sctp_getsockopt_connectx3(struct sock *sk, int len, if (len < sizeof(param32)) return -EINVAL; - if (copy_from_user(¶m32, optval, sizeof(param32))) - return -EFAULT; + param32 = *(struct compat_sctp_getaddrs_old *)param; - param.assoc_id = param32.assoc_id; - param.addr_num = param32.addr_num; - param.addrs = compat_ptr(param32.addrs); + param->assoc_id = param32.assoc_id; + param->addr_num = param32.addr_num; + param->addrs = compat_ptr(param32.addrs); } else #endif { - if (len < sizeof(param)) + if (len < sizeof(*param)) return -EINVAL; - if (copy_from_user(¶m, optval, sizeof(param))) - return -EFAULT; } - err = __sctp_setsockopt_connectx(sk, (struct sockaddr __user *) - param.addrs, param.addr_num, + addrs = memdup_user(param->addrs, param->addr_num);
I'm staring at this for a while now but I don't get this memdup_user. AFAICT, params->addrs is not __user anymore here, because sctp_getsockopt() copied the whole thing already, no? Also weird because it is being called from kernel_sctp_getsockopt(), which now has no knowledge of __user buffers. Maybe I didn't get something from the patch description.
+ if (IS_ERR(addrs))
+ return PTR_ERR(addrs);
+
+ err = __sctp_setsockopt_connectx(sk, addrs, param->addr_num,
&assoc_id);
+ kfree(addrs);
if (err == 0 || err == -EINPROGRESS) {
- if (copy_to_user(optval, &assoc_id, sizeof(assoc_id)))
- return -EFAULT;
- if (put_user(sizeof(assoc_id), optlen))
- return -EFAULT;
+ *(sctp_assoc_t *)param = assoc_id;
+ *optlen = sizeof(assoc_id);
}
return err;