* Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls @ 2020-12-07 10:55 François Legal 2020-12-11 6:55 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: François Legal @ 2020-12-07 10:55 UTC (permalink / raw) To: xenomai From: François LEGAL <devel@thom.fr.eu.org> Remove the copy of struct struct user_msghdr onto stack allocated buffer. Signed-off-by: François LEGAL <devel@thom.fr.eu.org> --- kernel/cobalt/posix/io.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/kernel/cobalt/posix/io.c b/kernel/cobalt/posix/io.c index f35aaf8..85272a5 100644 --- a/kernel/cobalt/posix/io.c +++ b/kernel/cobalt/posix/io.c @@ -79,18 +79,7 @@ COBALT_SYSCALL(write, handover, COBALT_SYSCALL(recvmsg, handover, (int fd, struct user_msghdr __user *umsg, int flags)) { - struct user_msghdr m; - ssize_t ret; - - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); - if (ret) - return ret; - - ret = rtdm_fd_recvmsg(fd, &m, flags); - if (ret < 0) - return ret; - - return cobalt_copy_to_user(umsg, &m, sizeof(*umsg)) ?: ret; + return rtdm_fd_recvmsg(fd, umsg, flags); } static int get_timespec(struct timespec *ts, @@ -123,12 +112,7 @@ COBALT_SYSCALL(recvmmsg, primary, COBALT_SYSCALL(sendmsg, handover, (int fd, struct user_msghdr __user *umsg, int flags)) { - struct user_msghdr m; - int ret; - - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); - - return ret ?: rtdm_fd_sendmsg(fd, &m, flags); + return rtdm_fd_sendmsg(fd, umsg, flags); } static int put_mmsglen(void __user **u_mmsg_p, const struct mmsghdr *mmsg) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls 2020-12-07 10:55 Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls François Legal @ 2020-12-11 6:55 ` Jan Kiszka 2020-12-11 8:05 ` François Legal 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2020-12-11 6:55 UTC (permalink / raw) To: François Legal, xenomai On 07.12.20 11:55, François Legal via Xenomai wrote: > From: François LEGAL <devel@thom.fr.eu.org> > > Remove the copy of struct struct user_msghdr onto stack allocated buffer. > Reasoning is missing here: The driver callbacks are supposed to do that copy-from-user. But the Question is: why? Is that local copy history left-over, or do only the drivers know how much to copy? Jan > Signed-off-by: François LEGAL <devel@thom.fr.eu.org> > --- > kernel/cobalt/posix/io.c | 20 ++------------------ > 1 file changed, 2 insertions(+), 18 deletions(-) > > diff --git a/kernel/cobalt/posix/io.c b/kernel/cobalt/posix/io.c > index f35aaf8..85272a5 100644 > --- a/kernel/cobalt/posix/io.c > +++ b/kernel/cobalt/posix/io.c > @@ -79,18 +79,7 @@ COBALT_SYSCALL(write, handover, > COBALT_SYSCALL(recvmsg, handover, > (int fd, struct user_msghdr __user *umsg, int flags)) > { > - struct user_msghdr m; > - ssize_t ret; > - > - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > - if (ret) > - return ret; > - > - ret = rtdm_fd_recvmsg(fd, &m, flags); > - if (ret < 0) > - return ret; > - > - return cobalt_copy_to_user(umsg, &m, sizeof(*umsg)) ?: ret; > + return rtdm_fd_recvmsg(fd, umsg, flags); > } > > static int get_timespec(struct timespec *ts, > @@ -123,12 +112,7 @@ COBALT_SYSCALL(recvmmsg, primary, > COBALT_SYSCALL(sendmsg, handover, > (int fd, struct user_msghdr __user *umsg, int flags)) > { > - struct user_msghdr m; > - int ret; > - > - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > - > - return ret ?: rtdm_fd_sendmsg(fd, &m, flags); > + return rtdm_fd_sendmsg(fd, umsg, flags); > } > > static int put_mmsglen(void __user **u_mmsg_p, const struct mmsghdr *mmsg) > > -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls 2020-12-11 6:55 ` Jan Kiszka @ 2020-12-11 8:05 ` François Legal 2020-12-11 8:36 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: François Legal @ 2020-12-11 8:05 UTC (permalink / raw) To: Jan Kiszka; +Cc: xenomai Le Vendredi, Décembre 11, 2020 07:55 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: > On 07.12.20 11:55, François Legal via Xenomai wrote: > > From: François LEGAL <devel@thom.fr.eu.org> > > > > Remove the copy of struct struct user_msghdr onto stack allocated buffer. > > > > Reasoning is missing here: The driver callbacks are supposed to do that > copy-from-user. > > But the Question is: why? Is that local copy history left-over, or do > only the drivers know how much to copy? > > Jan > Hi Jan, The drivers are effectively doing that, hence the problem that happens with SPECTRE mitigation activated (at least on arm arch). About the length of the copy, the drivers do about the the same, sizeof (struct user_msghdr) So now about the "WHY ?", that I can't say for sure. It might be history, as the other modified parts are IPCs and RTCAN (which I believe are older than RTNET integration). I dug this a little bit, and what I can say is that in 2.6.x, in rtdm/syscall.c, the copy_to/from_user calls were already present and in IPC, they were absent. François > > Signed-off-by: François LEGAL <devel@thom.fr.eu.org> > > --- > > kernel/cobalt/posix/io.c | 20 ++------------------ > > 1 file changed, 2 insertions(+), 18 deletions(-) > > > > diff --git a/kernel/cobalt/posix/io.c b/kernel/cobalt/posix/io.c > > index f35aaf8..85272a5 100644 > > --- a/kernel/cobalt/posix/io.c > > +++ b/kernel/cobalt/posix/io.c > > @@ -79,18 +79,7 @@ COBALT_SYSCALL(write, handover, > > COBALT_SYSCALL(recvmsg, handover, > > (int fd, struct user_msghdr __user *umsg, int flags)) > > { > > - struct user_msghdr m; > > - ssize_t ret; > > - > > - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > > - if (ret) > > - return ret; > > - > > - ret = rtdm_fd_recvmsg(fd, &m, flags); > > - if (ret < 0) > > - return ret; > > - > > - return cobalt_copy_to_user(umsg, &m, sizeof(*umsg)) ?: ret; > > + return rtdm_fd_recvmsg(fd, umsg, flags); > > } > > > > static int get_timespec(struct timespec *ts, > > @@ -123,12 +112,7 @@ COBALT_SYSCALL(recvmmsg, primary, > > COBALT_SYSCALL(sendmsg, handover, > > (int fd, struct user_msghdr __user *umsg, int flags)) > > { > > - struct user_msghdr m; > > - int ret; > > - > > - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > > - > > - return ret ?: rtdm_fd_sendmsg(fd, &m, flags); > > + return rtdm_fd_sendmsg(fd, umsg, flags); > > } > > > > static int put_mmsglen(void __user **u_mmsg_p, const struct mmsghdr *mmsg) > > > > > > -- > Siemens AG, T RDA IOT > Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls 2020-12-11 8:05 ` François Legal @ 2020-12-11 8:36 ` Jan Kiszka 2020-12-11 9:30 ` François Legal 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2020-12-11 8:36 UTC (permalink / raw) To: François Legal; +Cc: xenomai On 11.12.20 09:05, François Legal wrote: > Le Vendredi, Décembre 11, 2020 07:55 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: > >> On 07.12.20 11:55, François Legal via Xenomai wrote: >>> From: François LEGAL <devel@thom.fr.eu.org> >>> >>> Remove the copy of struct struct user_msghdr onto stack allocated buffer. >>> >> >> Reasoning is missing here: The driver callbacks are supposed to do that >> copy-from-user. >> >> But the Question is: why? Is that local copy history left-over, or do >> only the drivers know how much to copy? >> >> Jan >> > > Hi Jan, > > The drivers are effectively doing that, hence the problem that happens with SPECTRE mitigation activated (at least on arm arch). > About the length of the copy, the drivers do about the the same, sizeof (struct user_msghdr) Then we should move the common code into the common place, i.e. drop the copies in the drivers instead. > > So now about the "WHY ?", that I can't say for sure. It might be history, as the other modified parts are IPCs and RTCAN (which I believe are older than RTNET integration). I dug this a little bit, and what I can say is that in 2.6.x, in rtdm/syscall.c, the copy_to/from_user calls were already present and in IPC, they were absent. > Yeah, for a long time, we didn't notice such mess automatically... Jan > François > >>> Signed-off-by: François LEGAL <devel@thom.fr.eu.org> >>> --- >>> kernel/cobalt/posix/io.c | 20 ++------------------ >>> 1 file changed, 2 insertions(+), 18 deletions(-) >>> >>> diff --git a/kernel/cobalt/posix/io.c b/kernel/cobalt/posix/io.c >>> index f35aaf8..85272a5 100644 >>> --- a/kernel/cobalt/posix/io.c >>> +++ b/kernel/cobalt/posix/io.c >>> @@ -79,18 +79,7 @@ COBALT_SYSCALL(write, handover, >>> COBALT_SYSCALL(recvmsg, handover, >>> (int fd, struct user_msghdr __user *umsg, int flags)) >>> { >>> - struct user_msghdr m; >>> - ssize_t ret; >>> - >>> - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); >>> - if (ret) >>> - return ret; >>> - >>> - ret = rtdm_fd_recvmsg(fd, &m, flags); >>> - if (ret < 0) >>> - return ret; >>> - >>> - return cobalt_copy_to_user(umsg, &m, sizeof(*umsg)) ?: ret; >>> + return rtdm_fd_recvmsg(fd, umsg, flags); >>> } >>> >>> static int get_timespec(struct timespec *ts, >>> @@ -123,12 +112,7 @@ COBALT_SYSCALL(recvmmsg, primary, >>> COBALT_SYSCALL(sendmsg, handover, >>> (int fd, struct user_msghdr __user *umsg, int flags)) >>> { >>> - struct user_msghdr m; >>> - int ret; >>> - >>> - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); >>> - >>> - return ret ?: rtdm_fd_sendmsg(fd, &m, flags); >>> + return rtdm_fd_sendmsg(fd, umsg, flags); >>> } >>> >>> static int put_mmsglen(void __user **u_mmsg_p, const struct mmsghdr *mmsg) >>> >>> >> >> -- >> Siemens AG, T RDA IOT >> Corporate Competence Center Embedded Linux > -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls 2020-12-11 8:36 ` Jan Kiszka @ 2020-12-11 9:30 ` François Legal 2020-12-11 13:31 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: François Legal @ 2020-12-11 9:30 UTC (permalink / raw) To: Jan Kiszka; +Cc: xenomai Le Vendredi, Décembre 11, 2020 09:36 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: > On 11.12.20 09:05, François Legal wrote: > > Le Vendredi, Décembre 11, 2020 07:55 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: > > > >> On 07.12.20 11:55, François Legal via Xenomai wrote: > >>> From: François LEGAL <devel@thom.fr.eu.org> > >>> > >>> Remove the copy of struct struct user_msghdr onto stack allocated buffer. > >>> > >> > >> Reasoning is missing here: The driver callbacks are supposed to do that > >> copy-from-user. > >> > >> But the Question is: why? Is that local copy history left-over, or do > >> only the drivers know how much to copy? > >> > >> Jan > >> > > > > Hi Jan, > > > > The drivers are effectively doing that, hence the problem that happens with SPECTRE mitigation activated (at least on arm arch). > > About the length of the copy, the drivers do about the the same, sizeof (struct user_msghdr) > > Then we should move the common code into the common place, i.e. drop the > copies in the drivers instead. > I wonder, because from what I could read on RTIPC, these driver functions seems to be callable from kernel context (see rtipc_get_arg() ). There is a risk of trigging SPECTRE mitigation in case is is called from kernel no, or maybe it does not get called through posix/io.c ? François > > > > So now about the "WHY ?", that I can't say for sure. It might be history, as the other modified parts are IPCs and RTCAN (which I believe are older than RTNET integration). I dug this a little bit, and what I can say is that in 2.6.x, in rtdm/syscall.c, the copy_to/from_user calls were already present and in IPC, they were absent. > > > > Yeah, for a long time, we didn't notice such mess automatically... > > Jan > > > François > > > >>> Signed-off-by: François LEGAL <devel@thom.fr.eu.org> > >>> --- > >>> kernel/cobalt/posix/io.c | 20 ++------------------ > >>> 1 file changed, 2 insertions(+), 18 deletions(-) > >>> > >>> diff --git a/kernel/cobalt/posix/io.c b/kernel/cobalt/posix/io.c > >>> index f35aaf8..85272a5 100644 > >>> --- a/kernel/cobalt/posix/io.c > >>> +++ b/kernel/cobalt/posix/io.c > >>> @@ -79,18 +79,7 @@ COBALT_SYSCALL(write, handover, > >>> COBALT_SYSCALL(recvmsg, handover, > >>> (int fd, struct user_msghdr __user *umsg, int flags)) > >>> { > >>> - struct user_msghdr m; > >>> - ssize_t ret; > >>> - > >>> - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > >>> - if (ret) > >>> - return ret; > >>> - > >>> - ret = rtdm_fd_recvmsg(fd, &m, flags); > >>> - if (ret < 0) > >>> - return ret; > >>> - > >>> - return cobalt_copy_to_user(umsg, &m, sizeof(*umsg)) ?: ret; > >>> + return rtdm_fd_recvmsg(fd, umsg, flags); > >>> } > >>> > >>> static int get_timespec(struct timespec *ts, > >>> @@ -123,12 +112,7 @@ COBALT_SYSCALL(recvmmsg, primary, > >>> COBALT_SYSCALL(sendmsg, handover, > >>> (int fd, struct user_msghdr __user *umsg, int flags)) > >>> { > >>> - struct user_msghdr m; > >>> - int ret; > >>> - > >>> - ret = cobalt_copy_from_user(&m, umsg, sizeof(m)); > >>> - > >>> - return ret ?: rtdm_fd_sendmsg(fd, &m, flags); > >>> + return rtdm_fd_sendmsg(fd, umsg, flags); > >>> } > >>> > >>> static int put_mmsglen(void __user **u_mmsg_p, const struct mmsghdr *mmsg) > >>> > >>> > >> > >> -- > >> Siemens AG, T RDA IOT > >> Corporate Competence Center Embedded Linux > > > > -- > Siemens AG, T RDA IOT > Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls 2020-12-11 9:30 ` François Legal @ 2020-12-11 13:31 ` Jan Kiszka 0 siblings, 0 replies; 6+ messages in thread From: Jan Kiszka @ 2020-12-11 13:31 UTC (permalink / raw) To: François Legal; +Cc: xenomai On 11.12.20 10:30, François Legal wrote: > Le Vendredi, Décembre 11, 2020 09:36 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: > >> On 11.12.20 09:05, François Legal wrote: >>> Le Vendredi, Décembre 11, 2020 07:55 CET, Jan Kiszka <jan.kiszka@siemens.com> a écrit: >>> >>>> On 07.12.20 11:55, François Legal via Xenomai wrote: >>>>> From: François LEGAL <devel@thom.fr.eu.org> >>>>> >>>>> Remove the copy of struct struct user_msghdr onto stack allocated buffer. >>>>> >>>> >>>> Reasoning is missing here: The driver callbacks are supposed to do that >>>> copy-from-user. >>>> >>>> But the Question is: why? Is that local copy history left-over, or do >>>> only the drivers know how much to copy? >>>> >>>> Jan >>>> >>> >>> Hi Jan, >>> >>> The drivers are effectively doing that, hence the problem that happens with SPECTRE mitigation activated (at least on arm arch). >>> About the length of the copy, the drivers do about the the same, sizeof (struct user_msghdr) >> >> Then we should move the common code into the common place, i.e. drop the >> copies in the drivers instead. >> > > I wonder, because from what I could read on RTIPC, these driver functions seems to be callable from kernel context (see rtipc_get_arg() ). > There is a risk of trigging SPECTRE mitigation in case is is called from kernel no, or maybe it does not get called through posix/io.c ? RTDM drivers can call each other, and that can create a non-user entry to callbacks. rtipc_get_arg accounts for that via rtdm_fd_is_user(fd). If we consolidate again over "callbacks get user_msghdr from kernel memory" (but nothing that this struct my point to), no driver needs to copy that struct, just the syscall entry code. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-11 13:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-07 10:55 Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls François Legal 2020-12-11 6:55 ` Jan Kiszka 2020-12-11 8:05 ` François Legal 2020-12-11 8:36 ` Jan Kiszka 2020-12-11 9:30 ` François Legal 2020-12-11 13:31 ` Jan Kiszka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.