Re: Fwd: [Patch 1/5] Problems with upstream SPECTRE mitigation found in sendmsg/recvmsg syscalls

["François Legal" <devel@thom.fr.eu.org>]

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