On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:
Yeah, that seems reasonable. Here's the diff for that part:
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h index 7b6028b399d8..98bf19b4e086 100644 --- a/include/uapi/linux/seccomp.h +++ b/include/uapi/linux/seccomp.h @@ -118,7 +118,6 @@ struct seccomp_notif_resp { /**
- struct seccomp_notif_addfd
- @size: The size of the seccomp_notif_addfd datastructure
- @id: The ID of the seccomp notification
- @flags: SECCOMP_ADDFD_FLAG_*
- @srcfd: The local fd number
@@ -126,7 +125,6 @@ struct seccomp_notif_resp {
- @newfd_flags: The O_* flags the remote FD should have applied
*/ struct seccomp_notif_addfd {
- __u64 size; __u64 id; __u32 flags; __u32 srcfd;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 3c913f3b8451..00cbdad6c480 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1297,14 +1297,9 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter, struct seccomp_notif_addfd addfd; struct seccomp_knotif *knotif; struct seccomp_kaddfd kaddfd;
- u64 size; int ret;
- ret = get_user(size, &uaddfd->size);
- if (ret)
return ret;- ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
- ret = copy_from_user(&addfd, uaddfd, sizeof(addfd)); if (ret) return ret;
Looks good to me. If we ever change the size of this struct, we can do the work then to copy_struct_from_user.
+#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \
struct seccomp_notif_addfd)Lastly, what I believe to be a small mistake, it should be SECCOMP_IOW, based on the documentation in ioctl.h -- "_IOW means userland is writing and kernel is reading."
Oooooh. Yeah; good catch. Uhm, that means SECCOMP_IOCTL_NOTIF_ID_VALID is wrong too, yes? Tycho, Christian, how disruptive would this be to fix? (Perhaps support both and deprecate the IOR version at some point in the future?)
I think at a minimum we should change the uapi, and accept both (for now). Maybe a pr_warn_once telling people not to use the old one.
I can do the patch, if you want.
Diff for just addfd's change:
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h index 7b6028b399d8..98bf19b4e086 100644 --- a/include/uapi/linux/seccomp.h +++ b/include/uapi/linux/seccomp.h @@ -146,7 +144,7 @@ struct seccomp_notif_addfd { struct seccomp_notif_resp) #define SECCOMP_IOCTL_NOTIF_ID_VALID SECCOMP_IOR(2, __u64) /* On success, the return value is the remote process's added fd number */ -#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOR(3, \ +#define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOW(3, \ struct seccomp_notif_addfd) #endif /* _UAPI_LINUX_SECCOMP_H */
-- Kees Cook
Looks good. Thank you.