* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
[not found] <fd94b5e41a7c4edc8f743c56a04ed2c9@AcuMS.aculab.com>
@ 2020-05-20 23:33 ` kbuild test robot
2020-05-21 0:17 ` 'Marcelo Ricardo Leitner'
2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-05-20 23:33 UTC (permalink / raw)
To: David Laight, netdev, linux-sctp
Cc: kbuild-all, Neil Horman, 'Marcelo Ricardo Leitner'
[-- Attachment #1: Type: text/plain, Size: 8953 bytes --]
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v5.7-rc6 next-20200519]
[cannot apply to ipvs/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/David-Laight/sctp-Pull-the-user-copies-out-of-the-individual-sockopt-functions/20200521-040623
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4f65e2f483b6f764c15094d14dd53dda048a4048
config: i386-debian-10.3 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> net/sctp/socket.c:7187:5: warning: no previous prototype for 'kernel_sctp_getsockopt' [-Wmissing-prototypes]
int kernel_sctp_getsockopt(struct sock *sk, int optname, int len, void *optval,
^~~~~~~~~~~~~~~~~~~~~~
vim +/kernel_sctp_getsockopt +7187 net/sctp/socket.c
7186
> 7187 int kernel_sctp_getsockopt(struct sock *sk, int optname, int len, void *optval,
7188 int *optlen)
7189 {
7190 int retval;
7191
7192 lock_sock(sk);
7193
7194 switch (optname) {
7195 case SCTP_STATUS:
7196 retval = sctp_getsockopt_sctp_status(sk, len, optval, optlen);
7197 break;
7198 case SCTP_DISABLE_FRAGMENTS:
7199 retval = sctp_getsockopt_disable_fragments(sk, len, optval,
7200 optlen);
7201 break;
7202 case SCTP_EVENTS:
7203 retval = sctp_getsockopt_events(sk, len, optval, optlen);
7204 break;
7205 case SCTP_AUTOCLOSE:
7206 retval = sctp_getsockopt_autoclose(sk, len, optval, optlen);
7207 break;
7208 case SCTP_SOCKOPT_PEELOFF:
7209 retval = sctp_getsockopt_peeloff(sk, len, optval, optlen);
7210 break;
7211 case SCTP_SOCKOPT_PEELOFF_FLAGS:
7212 retval = sctp_getsockopt_peeloff_flags(sk, len, optval, optlen);
7213 break;
7214 case SCTP_PEER_ADDR_PARAMS:
7215 retval = sctp_getsockopt_peer_addr_params(sk, len, optval,
7216 optlen);
7217 break;
7218 case SCTP_DELAYED_SACK:
7219 retval = sctp_getsockopt_delayed_ack(sk, len, optval,
7220 optlen);
7221 break;
7222 case SCTP_INITMSG:
7223 retval = sctp_getsockopt_initmsg(sk, len, optval, optlen);
7224 break;
7225 case SCTP_GET_PEER_ADDRS:
7226 retval = sctp_getsockopt_peer_addrs(sk, len, optval,
7227 optlen);
7228 break;
7229 case SCTP_GET_LOCAL_ADDRS:
7230 retval = sctp_getsockopt_local_addrs(sk, len, optval,
7231 optlen);
7232 break;
7233 case SCTP_SOCKOPT_CONNECTX3:
7234 retval = sctp_getsockopt_connectx3(sk, len, optval, optlen);
7235 break;
7236 case SCTP_DEFAULT_SEND_PARAM:
7237 retval = sctp_getsockopt_default_send_param(sk, len,
7238 optval, optlen);
7239 break;
7240 case SCTP_DEFAULT_SNDINFO:
7241 retval = sctp_getsockopt_default_sndinfo(sk, len,
7242 optval, optlen);
7243 break;
7244 case SCTP_PRIMARY_ADDR:
7245 retval = sctp_getsockopt_primary_addr(sk, len, optval, optlen);
7246 break;
7247 case SCTP_NODELAY:
7248 retval = sctp_getsockopt_nodelay(sk, len, optval, optlen);
7249 break;
7250 case SCTP_RTOINFO:
7251 retval = sctp_getsockopt_rtoinfo(sk, len, optval, optlen);
7252 break;
7253 case SCTP_ASSOCINFO:
7254 retval = sctp_getsockopt_associnfo(sk, len, optval, optlen);
7255 break;
7256 case SCTP_I_WANT_MAPPED_V4_ADDR:
7257 retval = sctp_getsockopt_mappedv4(sk, len, optval, optlen);
7258 break;
7259 case SCTP_MAXSEG:
7260 retval = sctp_getsockopt_maxseg(sk, len, optval, optlen);
7261 break;
7262 case SCTP_GET_PEER_ADDR_INFO:
7263 retval = sctp_getsockopt_peer_addr_info(sk, len, optval,
7264 optlen);
7265 break;
7266 case SCTP_ADAPTATION_LAYER:
7267 retval = sctp_getsockopt_adaptation_layer(sk, len, optval,
7268 optlen);
7269 break;
7270 case SCTP_CONTEXT:
7271 retval = sctp_getsockopt_context(sk, len, optval, optlen);
7272 break;
7273 case SCTP_FRAGMENT_INTERLEAVE:
7274 retval = sctp_getsockopt_fragment_interleave(sk, len, optval,
7275 optlen);
7276 break;
7277 case SCTP_PARTIAL_DELIVERY_POINT:
7278 retval = sctp_getsockopt_partial_delivery_point(sk, len, optval,
7279 optlen);
7280 break;
7281 case SCTP_MAX_BURST:
7282 retval = sctp_getsockopt_maxburst(sk, len, optval, optlen);
7283 break;
7284 case SCTP_AUTH_KEY:
7285 case SCTP_AUTH_CHUNK:
7286 case SCTP_AUTH_DELETE_KEY:
7287 case SCTP_AUTH_DEACTIVATE_KEY:
7288 retval = -EOPNOTSUPP;
7289 break;
7290 case SCTP_HMAC_IDENT:
7291 retval = sctp_getsockopt_hmac_ident(sk, len, optval, optlen);
7292 break;
7293 case SCTP_AUTH_ACTIVE_KEY:
7294 retval = sctp_getsockopt_active_key(sk, len, optval, optlen);
7295 break;
7296 case SCTP_PEER_AUTH_CHUNKS:
7297 retval = sctp_getsockopt_peer_auth_chunks(sk, len, optval,
7298 optlen);
7299 break;
7300 case SCTP_LOCAL_AUTH_CHUNKS:
7301 retval = sctp_getsockopt_local_auth_chunks(sk, len, optval,
7302 optlen);
7303 break;
7304 case SCTP_GET_ASSOC_NUMBER:
7305 retval = sctp_getsockopt_assoc_number(sk, len, optval, optlen);
7306 break;
7307 case SCTP_GET_ASSOC_ID_LIST:
7308 retval = sctp_getsockopt_assoc_ids(sk, len, optval, optlen);
7309 break;
7310 case SCTP_AUTO_ASCONF:
7311 retval = sctp_getsockopt_auto_asconf(sk, len, optval, optlen);
7312 break;
7313 case SCTP_PEER_ADDR_THLDS:
7314 retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
7315 optlen, false);
7316 break;
7317 case SCTP_PEER_ADDR_THLDS_V2:
7318 retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
7319 optlen, true);
7320 break;
7321 case SCTP_GET_ASSOC_STATS:
7322 retval = sctp_getsockopt_assoc_stats(sk, len, optval, optlen);
7323 break;
7324 case SCTP_RECVRCVINFO:
7325 retval = sctp_getsockopt_recvrcvinfo(sk, len, optval, optlen);
7326 break;
7327 case SCTP_RECVNXTINFO:
7328 retval = sctp_getsockopt_recvnxtinfo(sk, len, optval, optlen);
7329 break;
7330 case SCTP_PR_SUPPORTED:
7331 retval = sctp_getsockopt_pr_supported(sk, len, optval, optlen);
7332 break;
7333 case SCTP_DEFAULT_PRINFO:
7334 retval = sctp_getsockopt_default_prinfo(sk, len, optval,
7335 optlen);
7336 break;
7337 case SCTP_PR_ASSOC_STATUS:
7338 retval = sctp_getsockopt_pr_assocstatus(sk, len, optval,
7339 optlen);
7340 break;
7341 case SCTP_PR_STREAM_STATUS:
7342 retval = sctp_getsockopt_pr_streamstatus(sk, len, optval,
7343 optlen);
7344 break;
7345 case SCTP_RECONFIG_SUPPORTED:
7346 retval = sctp_getsockopt_reconfig_supported(sk, len, optval,
7347 optlen);
7348 break;
7349 case SCTP_ENABLE_STREAM_RESET:
7350 retval = sctp_getsockopt_enable_strreset(sk, len, optval,
7351 optlen);
7352 break;
7353 case SCTP_STREAM_SCHEDULER:
7354 retval = sctp_getsockopt_scheduler(sk, len, optval,
7355 optlen);
7356 break;
7357 case SCTP_STREAM_SCHEDULER_VALUE:
7358 retval = sctp_getsockopt_scheduler_value(sk, len, optval,
7359 optlen);
7360 break;
7361 case SCTP_INTERLEAVING_SUPPORTED:
7362 retval = sctp_getsockopt_interleaving_supported(sk, len, optval,
7363 optlen);
7364 break;
7365 case SCTP_REUSE_PORT:
7366 retval = sctp_getsockopt_reuse_port(sk, len, optval, optlen);
7367 break;
7368 case SCTP_EVENT:
7369 retval = sctp_getsockopt_event(sk, len, optval, optlen);
7370 break;
7371 case SCTP_ASCONF_SUPPORTED:
7372 retval = sctp_getsockopt_asconf_supported(sk, len, optval,
7373 optlen);
7374 break;
7375 case SCTP_AUTH_SUPPORTED:
7376 retval = sctp_getsockopt_auth_supported(sk, len, optval,
7377 optlen);
7378 break;
7379 case SCTP_ECN_SUPPORTED:
7380 retval = sctp_getsockopt_ecn_supported(sk, len, optval, optlen);
7381 break;
7382 case SCTP_EXPOSE_POTENTIALLY_FAILED_STATE:
7383 retval = sctp_getsockopt_pf_expose(sk, len, optval, optlen);
7384 break;
7385 default:
7386 retval = -ENOPROTOOPT;
7387 break;
7388 }
7389
7390 release_sock(sk);
7391 return retval;
7392 }
7393
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34553 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
[not found] <fd94b5e41a7c4edc8f743c56a04ed2c9@AcuMS.aculab.com>
2020-05-20 23:33 ` [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions kbuild test robot
@ 2020-05-21 0:17 ` 'Marcelo Ricardo Leitner'
2020-05-21 7:32 ` David Laight
2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
2 siblings, 1 reply; 9+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2020-05-21 0:17 UTC (permalink / raw)
To: David Laight; +Cc: netdev, linux-sctp, Neil Horman
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().
...
> @@ -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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 0:17 ` 'Marcelo Ricardo Leitner'
@ 2020-05-21 7:32 ` David Laight
2020-05-21 14:36 ` 'Marcelo Ricardo Leitner'
0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2020-05-21 7:32 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'; +Cc: netdev, linux-sctp, Neil Horman
From: 'Marcelo Ricardo Leitner'
> Sent: 21 May 2020 01:17
> 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().
> ...
> > @@ -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
..
> > } 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.
The connectx3 sockopt buffer contains a pointer to the user buffer
that contains the actual addresses.
So a second copy_from_user() is needed.
This does mean that this option can only be actioned from userspace.
Kernel code can get the same functionality using one of the
other interfaces to connectx().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 7:32 ` David Laight
@ 2020-05-21 14:36 ` 'Marcelo Ricardo Leitner'
0 siblings, 0 replies; 9+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2020-05-21 14:36 UTC (permalink / raw)
To: David Laight; +Cc: netdev, linux-sctp, Neil Horman
On Thu, May 21, 2020 at 07:32:14AM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 21 May 2020 01:17
> > 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().
> > ...
> > > @@ -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
> ..
> > > } 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.
>
> The connectx3 sockopt buffer contains a pointer to the user buffer
> that contains the actual addresses.
> So a second copy_from_user() is needed.
Oh, I see now. Thanks.
>
> This does mean that this option can only be actioned from userspace.
>
> Kernel code can get the same functionality using one of the
> other interfaces to connectx().
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
[not found] <fd94b5e41a7c4edc8f743c56a04ed2c9@AcuMS.aculab.com>
2020-05-20 23:33 ` [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions kbuild test robot
2020-05-21 0:17 ` 'Marcelo Ricardo Leitner'
@ 2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
2020-05-21 15:39 ` Christoph Hellwig
2020-05-21 16:09 ` David Laight
2 siblings, 2 replies; 9+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2020-05-21 15:37 UTC (permalink / raw)
To: David Laight; +Cc: netdev, linux-sctp, Neil Horman, Christoph Hellwig
On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
I wish we could split this patch into multiple ones. Like, one for
each sockopt, but it doesn't seem possible. It's tough to traverse
trough 5k lines long patch. :-(
> Since SCTP rather abuses getsockopt() to perform operations and uses
> the user buffer to select the association to get values from
> the sctp_getsockopt() has to do a Read-Modify-Write on the user buffer.
>
> An on-stack buffer is used for short requests this allows the length
> check for simple getsockopt requests to be done by the wrapper.
>
> Signed-off-by: David Laight <david.laight@aculab.com>
>
> --
>
> While this patch might make it easier to export the functionality
> to other kernel modules, it doesn't make that change.
>
> 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().
It should have been a linear buffer. The secondary __user access is
way worse than having the application to do another allocation. But
too late..
>
> The SCTP_SOCKOPT_PEELOFF getsockopt() (another abuse) also wants to
> return a positive value and a buffer (containing the same value) on
> success.
Unnecessary, agree, but too late for changing that.
>
> Both these stop the sctp_getsockopt_xxx() functions returning
> 'error or length'.
>
> There is also real fubar of SCTP_GET_LOCAL_ADDRS which has to
> return the wrong length 'for historic compatibility'.
> Although I'm not sure how portable that makes applications.
>
> Reduces the code by about 800 lines and 8k bytes (x86-64).
> Most of the changed lines are replacing x.y with x->y and
> simplifying error paths.
This cleanup is something that I've been longing for a while now.
Avoiding these repetitive user space handling is very welcomed.
Also, I think this is pretty much aligned with Christoph's goal as
well and can make the patches in his series easier/cleaner.
Other than the comments here, this patch LGTM.
>
> Passes 'sparse' and at least some options work.
Assuming a v2 is coming, to appease the buildbot :)
...
> +static int sctp_setsockopt(struct sock *sk, int level, int optname,
> + char __user *u_optval, unsigned int optlen)
> +{
> + u64 param_buf[8];
> + int retval = 0;
> + void *optval;
> +
> + pr_debug("%s: sk:%p, optname:%d\n", __func__, sk, optname);
> +
> + /* I can hardly begin to describe how wrong this is. This is
> + * so broken as to be worse than useless. The API draft
> + * REALLY is NOT helpful here... I am not convinced that the
> + * semantics of setsockopt() with a level OTHER THAN SOL_SCTP
> + * are at all well-founded.
> + */
> + if (level != SOL_SCTP) {
> + struct sctp_af *af = sctp_sk(sk)->pf->af;
> + return af->setsockopt(sk, level, optname, u_optval, optlen);
> + }
> +
> + if (optlen < sizeof (param_buf)) {
> + if (copy_from_user(¶m_buf, u_optval, optlen))
> + return -EFAULT;
> + optval = param_buf;
> + } else {
> + if (optlen > USHRT_MAX)
> + optlen = USHRT_MAX;
There are functions that can work with and expect buffers larger than
that, such as sctp_setsockopt_auth_key:
@@ -3693,10 +3588,6 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
*/
optlen = min_t(unsigned int, optlen, USHRT_MAX + sizeof(*authkey));
and sctp_setsockopt_reset_streams:
/* srs_number_streams is u16, so optlen can't be bigger than this. */
optlen = min_t(unsigned int, optlen, USHRT_MAX +
sizeof(__u16) * sizeof(*params));
Need to cope with those here.
> + optval = memdup_user(u_optval, optlen);
> + if (IS_ERR(optval))
> + return PTR_ERR(optval);
> + }
> +
> + retval = kernel_sctp_setsockopt(sk, optname, optval, optlen);
> + if (optval != param_buf)
> + kfree(optval);
> +
> return retval;
> }
>
...
> +static int sctp_getsockopt(struct sock *sk, int level, int optname,
> + char __user *u_optval, int __user *u_optlen)
> +{
> + u64 param_buf[8];
> + int retval = 0;
> + void *optval;
> + int len, optlen;
> +
> + pr_debug("%s: sk:%p, optname:%d\n", __func__, sk, optname);
> +
> + /* I can hardly begin to describe how wrong this is. This is
> + * so broken as to be worse than useless. The API draft
> + * REALLY is NOT helpful here... I am not convinced that the
> + * semantics of getsockopt() with a level OTHER THAN SOL_SCTP
> + * are at all well-founded.
> + */
> + if (level != SOL_SCTP) {
> + struct sctp_af *af = sctp_sk(sk)->pf->af;
> +
> + retval = af->getsockopt(sk, level, optname, u_optval, u_optlen);
> + return retval;
> + }
> +
> + if (get_user(len, u_optlen))
> + return -EFAULT;
> +
> + if (len < 0)
> + return -EINVAL;
> +
> + /* Many options are RMW so we must read in the user buffer.
> + * For safetly we need to initialise it to avoid leaking
> + * kernel data - the copy does this as well.
> + * To simplify the processing of simple options the buffer length
> + * check is repeated after the request is actioned.
> + */
> + if (len < sizeof (param_buf)) {
> + /* Zero first bytes to stop KASAN complaining. */
> + param_buf[0] = 0;
> + if (copy_from_user(¶m_buf, u_optval, len))
> + return -EFAULT;
> + optval = param_buf;
> + } else {
> + if (len > USHRT_MAX)
> + len = USHRT_MAX;
This limit is not present today for sctp_getsockopt_local_addrs()
calls (there may be others). As is, it will limit it and may mean
that it can't dump all addresses. We have discussed this and didn't
come to a conclusion on what is a safe limit to use here, at least not
on that time.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
@ 2020-05-21 15:39 ` Christoph Hellwig
2020-05-21 15:52 ` David Laight
2020-05-21 16:09 ` David Laight
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-05-21 15:39 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: David Laight, netdev, linux-sctp, Neil Horman, Christoph Hellwig
On Thu, May 21, 2020 at 12:37:29PM -0300, 'Marcelo Ricardo Leitner' wrote:
> On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
>
> I wish we could split this patch into multiple ones. Like, one for
> each sockopt, but it doesn't seem possible. It's tough to traverse
> trough 5k lines long patch. :-(
I have a series locally that started this, I an try to resurrect and
finish it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 15:39 ` Christoph Hellwig
@ 2020-05-21 15:52 ` David Laight
0 siblings, 0 replies; 9+ messages in thread
From: David Laight @ 2020-05-21 15:52 UTC (permalink / raw)
To: 'Christoph Hellwig', 'Marcelo Ricardo Leitner'
Cc: netdev, linux-sctp, Neil Horman
From: Christoph Hellwig
> Sent: 21 May 2020 16:40
> On Thu, May 21, 2020 at 12:37:29PM -0300, 'Marcelo Ricardo Leitner' wrote:
> > On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
> >
> > I wish we could split this patch into multiple ones. Like, one for
> > each sockopt, but it doesn't seem possible. It's tough to traverse
> > trough 5k lines long patch. :-(
>
> I have a series locally that started this, I an try to resurrect and
> finish it.
It is almost better just to look at the new file.
Possibly in a 'side by side' diff.
The only real split is getsockopt v setsockopt.
But they are in separated in the patch anyway.
Most of the patch is just mechanical changes to pass
the compiler and sparse.
Still took over a day to write.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
2020-05-21 15:39 ` Christoph Hellwig
@ 2020-05-21 16:09 ` David Laight
2020-05-21 16:45 ` 'Marcelo Ricardo Leitner'
1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2020-05-21 16:09 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: netdev, linux-sctp, Neil Horman, Christoph Hellwig
From: 'Marcelo Ricardo Leitner'
> Sent: 21 May 2020 16:37
> 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().
>
> It should have been a linear buffer. The secondary __user access is
> way worse than having the application to do another allocation. But
> too late..
I think that is SCTP_SOCKOPT_CONNECTX ?
> Other than the comments here, this patch LGTM.
Thanks.
> Assuming a v2 is coming, to appease the buildbot :)
I'd got an definition in sctp.h and an EXPORT() but took them out.
I'll also increase the setsockopt limit.
...
> > + if (optlen < sizeof (param_buf)) {
> > + if (copy_from_user(¶m_buf, u_optval, optlen))
> > + return -EFAULT;
> > + optval = param_buf;
> > + } else {
> > + if (optlen > USHRT_MAX)
> > + optlen = USHRT_MAX;
>
> There are functions that can work with and expect buffers larger than
> that, such as sctp_setsockopt_auth_key:
I'd assumed the maximums were silly.
But a few more than 64k is enough, the lengths are in bytes.
OTOH 128k is a nice round limit - and plenty AFAICT.
...
> > + if (len < sizeof (param_buf)) {
> > + /* Zero first bytes to stop KASAN complaining. */
> > + param_buf[0] = 0;
> > + if (copy_from_user(¶m_buf, u_optval, len))
> > + return -EFAULT;
> > + optval = param_buf;
> > + } else {
> > + if (len > USHRT_MAX)
> > + len = USHRT_MAX;
>
> This limit is not present today for sctp_getsockopt_local_addrs()
> calls (there may be others). As is, it will limit it and may mean
> that it can't dump all addresses. We have discussed this and didn't
> come to a conclusion on what is a safe limit to use here, at least not
> on that time.
It needs some limit. memdup_user() might limit at 32MB.
I couldn't decide is some of the allocators limit it further.
In any case an IPv6 address is what? under 128 bytes.
64k is 512 address, things are going to explode elsewhere first.
I didn't see 'get' requests that did 64k + a bit.
It should be possible to loop using a larger kernel buffer if the
data won't fit.
Doable as a later patch to avoid complications.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
2020-05-21 16:09 ` David Laight
@ 2020-05-21 16:45 ` 'Marcelo Ricardo Leitner'
0 siblings, 0 replies; 9+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2020-05-21 16:45 UTC (permalink / raw)
To: David Laight; +Cc: netdev, linux-sctp, Neil Horman, Christoph Hellwig
On Thu, May 21, 2020 at 04:09:15PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 21 May 2020 16:37
> > 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().
> >
> > It should have been a linear buffer. The secondary __user access is
> > way worse than having the application to do another allocation. But
> > too late..
>
> I think that is SCTP_SOCKOPT_CONNECTX ?
Right :-)
...
> > > + if (optlen < sizeof (param_buf)) {
> > > + if (copy_from_user(¶m_buf, u_optval, optlen))
> > > + return -EFAULT;
> > > + optval = param_buf;
> > > + } else {
> > > + if (optlen > USHRT_MAX)
> > > + optlen = USHRT_MAX;
> >
> > There are functions that can work with and expect buffers larger than
> > that, such as sctp_setsockopt_auth_key:
>
> I'd assumed the maximums were silly.
> But a few more than 64k is enough, the lengths are in bytes.
> OTOH 128k is a nice round limit - and plenty AFAICT.
LGTM too.
>
> ...
> > > + if (len < sizeof (param_buf)) {
> > > + /* Zero first bytes to stop KASAN complaining. */
> > > + param_buf[0] = 0;
> > > + if (copy_from_user(¶m_buf, u_optval, len))
> > > + return -EFAULT;
> > > + optval = param_buf;
> > > + } else {
> > > + if (len > USHRT_MAX)
> > > + len = USHRT_MAX;
> >
> > This limit is not present today for sctp_getsockopt_local_addrs()
> > calls (there may be others). As is, it will limit it and may mean
> > that it can't dump all addresses. We have discussed this and didn't
> > come to a conclusion on what is a safe limit to use here, at least not
> > on that time.
>
> It needs some limit. memdup_user() might limit at 32MB.
> I couldn't decide is some of the allocators limit it further.
> In any case an IPv6 address is what? under 128 bytes.
> 64k is 512 address, things are going to explode elsewhere first.
If it does, we probably can fix that too.
>
> I didn't see 'get' requests that did 64k + a bit.
>
> It should be possible to loop using a larger kernel buffer if the
> data won't fit.
> Doable as a later patch to avoid complications.
Sounds complicated. 128k should be more than enough here as well.
sctp_getsockopt_local_addrs() will adjust the output to fit on the
buffer. Point being, with enough buffer, it will support the limits
the RFC states, and if the user supplies a smaller buffer, it will
dump what it can. If the user pass a larger buffer, it doesn't need
it, and it's safe to ignore the rest of the buffer (as the patch is
doing here). I didn't check the other functions now, though.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-21 16:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <fd94b5e41a7c4edc8f743c56a04ed2c9@AcuMS.aculab.com>
2020-05-20 23:33 ` [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions kbuild test robot
2020-05-21 0:17 ` 'Marcelo Ricardo Leitner'
2020-05-21 7:32 ` David Laight
2020-05-21 14:36 ` 'Marcelo Ricardo Leitner'
2020-05-21 15:37 ` 'Marcelo Ricardo Leitner'
2020-05-21 15:39 ` Christoph Hellwig
2020-05-21 15:52 ` David Laight
2020-05-21 16:09 ` David Laight
2020-05-21 16:45 ` 'Marcelo Ricardo Leitner'
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).