| 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
| ||
|
Message-ID: <CA+CK2bD68E1-AWxz9p-Byyb=fDVQGu4Q+GpW2ogCNdjCxbAJqQ@mail.gmail.com>
Date: Mon, 22 Sep 2025 17:09:11 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: pratyush@...nel.org, jasonmiu@...gle.com, graf@...zon.com,
changyuanl@...gle.com, rppt@...nel.org, dmatlack@...gle.com,
rientjes@...gle.com, corbet@....net, rdunlap@...radead.org,
ilpo.jarvinen@...ux.intel.com, kanie@...ux.alibaba.com, ojeda@...nel.org,
aliceryhl@...gle.com, masahiroy@...nel.org, akpm@...ux-foundation.org,
tj@...nel.org, yoann.congal@...le.fr, mmaurer@...gle.com,
roman.gushchin@...ux.dev, chenridong@...wei.com, axboe@...nel.dk,
mark.rutland@....com, jannh@...gle.com, vincent.guittot@...aro.org,
hannes@...xchg.org, dan.j.williams@...el.com, david@...hat.com,
joel.granados@...nel.org, rostedt@...dmis.org, anna.schumaker@...cle.com,
song@...nel.org, zhangguopeng@...inos.cn, linux@...ssschuh.net,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, linux-mm@...ck.org,
gregkh@...uxfoundation.org, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
rafael@...nel.org, dakr@...nel.org, bartosz.golaszewski@...aro.org,
cw00.choi@...sung.com, myungjoo.ham@...sung.com, yesanishhere@...il.com,
Jonathan.Cameron@...wei.com, quic_zijuhu@...cinc.com,
aleksander.lobakin@...el.com, ira.weiny@...el.com,
andriy.shevchenko@...ux.intel.com, leon@...nel.org, lukas@...ner.de,
bhelgaas@...gle.com, wagi@...nel.org, djeffery@...hat.com,
stuart.w.hayes@...il.com, ptyadav@...zon.de, lennart@...ttering.net,
brauner@...nel.org, linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
saeedm@...dia.com, ajayachandra@...dia.com, parav@...dia.com,
leonro@...dia.com, witu@...dia.com
Subject: Re: [PATCH v3 16/30] liveupdate: luo_ioctl: add userpsace interface
> > + * - EINVAL: Everything about the IOCTL was understood, but a field is not
> > + * correct.
> > + * - ENOENT: An ID or IOVA provided does not exist.
> ^^^^^^^^^
>
> Maybe this should be 'token' ?
Yes, replaced with token. :-)
> > +struct liveupdate_ioctl_fd_unpreserve {
> > + __u32 size;
> > + __aligned_u64 token;
> > +};
>
> It is best to explicitly pad, so add a __u32 reserved between size and
> token
>
> Then you need to also check that the reserved is 0 when parsing it,
> return -EOPNOTSUPP otherwise.
Done.
>
> > +static atomic_t luo_device_in_use = ATOMIC_INIT(0);
>
> I suggest you bundle this together into one struct with the misc_dev
> and the other globals and largely pretend it is not global, eg refer
> to it through container_of, etc
>
> Following practices like this make it harder to abuse the globals.
Done, good suggestion.
> > +struct luo_ucmd {
> > + void __user *ubuffer;
> > + u32 user_size;
> > + void *cmd;
> > +};
> > +
> > +static int luo_ioctl_fd_preserve(struct luo_ucmd *ucmd)
> > +{
> > + struct liveupdate_ioctl_fd_preserve *argp = ucmd->cmd;
> > + int ret;
> > +
> > + ret = luo_register_file(argp->token, argp->fd);
> > + if (!ret)
> > + return ret;
> > +
> > + if (copy_to_user(ucmd->ubuffer, argp, ucmd->user_size))
> > + return -EFAULT;
>
> This will overflow memory, ucmd->user_size may be > sizeof(*argp)
>
> The respond function is an important part of this scheme:
>
> static inline int iommufd_ucmd_respond(struct iommufd_ucmd *ucmd,
> size_t cmd_len)
> {
> if (copy_to_user(ucmd->ubuffer, ucmd->cmd,
> min_t(size_t, ucmd->user_size, cmd_len)))
> return -EFAULT;
>
> The min (sizeof(*argp) in this case) can't be skipped!
Done, thank you for catching this.
> > +static int luo_ioctl_fd_restore(struct luo_ucmd *ucmd)
> > +{
> > + struct liveupdate_ioctl_fd_restore *argp = ucmd->cmd;
> > + struct file *file;
> > + int ret;
> > +
> > + argp->fd = get_unused_fd_flags(O_CLOEXEC);
> > + if (argp->fd < 0) {
> > + pr_err("Failed to allocate new fd: %d\n", argp->fd);
>
> No need
Removed
> > + return argp->fd;
> > + }
> > +
> > + ret = luo_retrieve_file(argp->token, &file);
> > + if (ret < 0) {
> > + put_unused_fd(argp->fd);
> > +
> > + return ret;
> > + }
> > +
> > + fd_install(argp->fd, file);
> > +
> > + if (copy_to_user(ucmd->ubuffer, argp, ucmd->user_size))
> > + return -EFAULT;
>
> Wrong order, fd_install must be last right before return 0. Failing
> system calls should not leave behind installed FDs.
Fixed.
>
> > +static int luo_ioctl_set_event(struct luo_ucmd *ucmd)
> > +{
> > + struct liveupdate_ioctl_set_event *argp = ucmd->cmd;
> > + int ret;
> > +
> > + switch (argp->event) {
> > + case LIVEUPDATE_PREPARE:
> > + ret = luo_prepare();
> > + break;
> > + case LIVEUPDATE_FINISH:
> > + ret = luo_finish();
> > + break;
> > + case LIVEUPDATE_CANCEL:
> > + ret = luo_cancel();
> > + break;
> > + default:
> > + ret = -EINVAL;
>
> EOPNOTSUPP
Ack.
>
> > +union ucmd_buffer {
> > + struct liveupdate_ioctl_fd_preserve preserve;
> > + struct liveupdate_ioctl_fd_unpreserve unpreserve;
> > + struct liveupdate_ioctl_fd_restore restore;
> > + struct liveupdate_ioctl_get_state state;
> > + struct liveupdate_ioctl_set_event event;
> > +};
>
> I discourage the column alignment. Also sort by name.
Done
>
> > +static const struct luo_ioctl_op luo_ioctl_ops[] = {
> > + IOCTL_OP(LIVEUPDATE_IOCTL_FD_PRESERVE, luo_ioctl_fd_preserve,
> > + struct liveupdate_ioctl_fd_preserve, token),
> > + IOCTL_OP(LIVEUPDATE_IOCTL_FD_UNPRESERVE, luo_ioctl_fd_unpreserve,
> > + struct liveupdate_ioctl_fd_unpreserve, token),
> > + IOCTL_OP(LIVEUPDATE_IOCTL_FD_RESTORE, luo_ioctl_fd_restore,
> > + struct liveupdate_ioctl_fd_restore, token),
> > + IOCTL_OP(LIVEUPDATE_IOCTL_GET_STATE, luo_ioctl_get_state,
> > + struct liveupdate_ioctl_get_state, state),
> > + IOCTL_OP(LIVEUPDATE_IOCTL_SET_EVENT, luo_ioctl_set_event,
> > + struct liveupdate_ioctl_set_event, event),
>
> Sort by name
Done
Powered by blists - more mailing lists