The results of "access_ok()" can be mis-speculated. The result is that
you can end speculatively:
if (access_ok(from, size))
// Right here
For the same reason as done in copy_from_user() by
commit 74e19ef0ff80 ("uaccess: Add speculation barrier to
copy_from_user()"), add a speculation barrier to copy_from_user_iter().
See commit 74e19ef0ff80 ("uaccess: Add speculation barrier to
copy_from_user()") for more details.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
lib/iov_iter.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f9193f952f49..ebf524a37907 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -50,6 +50,13 @@ size_t copy_from_user_iter(void __user *iter_from, size_t progress,
if (should_fail_usercopy())
return len;
if (access_ok(iter_from, len)) {
+ /*
+ * Ensure that bad access_ok() speculation will not
+ * lead to nasty side effects *after* the copy is
+ * finished:
+ */
+ barrier_nospec();
+
to += progress;
instrument_copy_from_user_before(to, iter_from, len);
res = raw_copy_from_user(to, iter_from, len);
--
2.49.0On Sun, 22 Jun 2025 11:52:40 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> The results of "access_ok()" can be mis-speculated. The result is that
> you can end speculatively:
>
> if (access_ok(from, size))
> // Right here
>
> For the same reason as done in copy_from_user() by
> commit 74e19ef0ff80 ("uaccess: Add speculation barrier to
> copy_from_user()"), add a speculation barrier to copy_from_user_iter().
I'm sure I sent a patch to change this code to used the 'masked' functions.
Probably ought to be done at the same time.
Would have been early feb, about the time I suggested:
+#ifdef masked_user_access_begin
+#define masked_user_read_access_begin(from, size) \
+ ((*(from) = masked_user_access_begin(*(from))), 1)
+#define masked_user_write_access_begin(from, size) \
+ ((*(from) = masked_user_access_begin(*(from))), 1)
+#else
+#define masked_user_read_access_begin(from, size) \
+ user_read_access_begin(*(from), size)
+#define masked_user_write_access_begin(from, size) \
+ user_write_access_begin(*(from), size)
+#endif
allowing:
- if (!user_read_access_begin(from, sizeof(*from)))
+ if (!masked_user_read_access_begin(&from, sizeof(*from)))
David
>
> See commit 74e19ef0ff80 ("uaccess: Add speculation barrier to
> copy_from_user()") for more details.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> lib/iov_iter.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index f9193f952f49..ebf524a37907 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -50,6 +50,13 @@ size_t copy_from_user_iter(void __user *iter_from, size_t progress,
> if (should_fail_usercopy())
> return len;
> if (access_ok(iter_from, len)) {
> + /*
> + * Ensure that bad access_ok() speculation will not
> + * lead to nasty side effects *after* the copy is
> + * finished:
> + */
> + barrier_nospec();
> +
> to += progress;
> instrument_copy_from_user_before(to, iter_from, len);
> res = raw_copy_from_user(to, iter_from, len);
On Sun, 22 Jun 2025 at 02:52, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> The results of "access_ok()" can be mis-speculated.
Hmm. This code is critical. I think it should be converted to use that
masked address thing if we have to add it here.
And at some point this access_ok() didn't even exist, because we check
the addresses at iter creation time. So this one might be a "belt and
suspenders" check, rather than something critical.
(Although I also suspect that when we added ITER_UBUF we might have
created cases where those user addresses aren't checked at iter
creation time any more).
Linus
Le 22/06/2025 à 18:57, Linus Torvalds a écrit :
> On Sun, 22 Jun 2025 at 02:52, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> The results of "access_ok()" can be mis-speculated.
>
> Hmm. This code is critical. I think it should be converted to use that
> masked address thing if we have to add it here.
Ok, I'll add it.
>
> And at some point this access_ok() didn't even exist, because we check
> the addresses at iter creation time. So this one might be a "belt and
> suspenders" check, rather than something critical.
>
> (Although I also suspect that when we added ITER_UBUF we might have
> created cases where those user addresses aren't checked at iter
> creation time any more).
>
Let's take the follow path as an exemple:
snd_pcm_ioctl(SNDRV_PCM_IOCTL_WRITEI_FRAMES)
snd_pcm_common_ioctl()
snd_pcm_xferi_frames_ioctl()
snd_pcm_lib_write()
__snd_pcm_lib_xfer()
default_write_copy()
copy_from_iter()
_copy_from_iter()
__copy_from_iter()
iterate_and_advance()
iterate_and_advance2()
iterate_iovec()
copy_from_user_iter()
As far as I can see, none of those functions check the accessibility of
the iovec. Am I missing something ?
Christophe
On Mon, 23 Jun 2025 at 22:49, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> >
> > (Although I also suspect that when we added ITER_UBUF we might have
> > created cases where those user addresses aren't checked at iter
> > creation time any more).
> >
>
> Let's take the follow path as an exemple:
>
> snd_pcm_ioctl(SNDRV_PCM_IOCTL_WRITEI_FRAMES)
> snd_pcm_common_ioctl()
> snd_pcm_xferi_frames_ioctl()
> snd_pcm_lib_write()
> __snd_pcm_lib_xfer()
> default_write_copy()
> copy_from_iter()
> _copy_from_iter()
> __copy_from_iter()
> iterate_and_advance()
> iterate_and_advance2()
> iterate_iovec()
> copy_from_user_iter()
>
> As far as I can see, none of those functions check the accessibility of
> the iovec. Am I missing something ?
So we still to do this checking at creation time (see import_iovec ->
__import_iovec, and import_ubuf).
In the path you give as an example, the check happens at that
"do_transfer()" stage when it does
err = import_ubuf(type, (__force void __user *)data, bytes, &iter);
but yeah, it's very non-obvious (see __snd_pcm_lib_xfer(), which calls
writer() which is either interleaved_copy() or noninterleaved_copy(),
and then they do that do_transfer() thing which does that
import_ubuf() thing.
So *because* you were supposed to have checked your iov_iters
beforehand, the actual iter code itself at some point just used
__copy_to_user() directly with no checking at all.
And that all was really *much* too subtle, and Al fixed this a few
years ago (see commit 09fc68dc66f7: "iov_iter: saner checks on
copyin/copyout")
Linus
On Tue, 24 Jun 2025 07:49:03 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Le 22/06/2025 à 18:57, Linus Torvalds a écrit : > > On Sun, 22 Jun 2025 at 02:52, Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> The results of "access_ok()" can be mis-speculated. > > > > Hmm. This code is critical. I think it should be converted to use that > > masked address thing if we have to add it here. > > Ok, I'll add it. > > > > > And at some point this access_ok() didn't even exist, because we check > > the addresses at iter creation time. So this one might be a "belt and > > suspenders" check, rather than something critical. > > > > (Although I also suspect that when we added ITER_UBUF we might have > > created cases where those user addresses aren't checked at iter > > creation time any more). > > > > Let's take the follow path as an exemple: > > snd_pcm_ioctl(SNDRV_PCM_IOCTL_WRITEI_FRAMES) > snd_pcm_common_ioctl() > snd_pcm_xferi_frames_ioctl() > snd_pcm_lib_write() > __snd_pcm_lib_xfer() > default_write_copy() > copy_from_iter() > _copy_from_iter() > __copy_from_iter() > iterate_and_advance() > iterate_and_advance2() > iterate_iovec() > copy_from_user_iter() > > As far as I can see, none of those functions check the accessibility of > the iovec. Am I missing something ? The import_ubuf() in do_transfer() ought to contain one. But really you want the one in copy_from_user_iter() rather than the outer one. Mind you that code is horrid. The code only ever copies a single buffer, so could be much shorter. And is that deep call chain really needed for the very common case of one buffer. David > > Christophe
On Sun, 22 Jun 2025 09:57:20 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, 22 Jun 2025 at 02:52, Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: > > > > The results of "access_ok()" can be mis-speculated. > > Hmm. This code is critical. I think it should be converted to use that > masked address thing if we have to add it here. If access_ok() is mis-speculated then you get a read from the user-specified kernel address - I don't think that matters. The hacker would need to find somewhere where the read value was used in a test or memory access so that side effects (typically cache line evictions) can be detected. But copy_from_user_iter() is pretty much always used for 'data' not 'control pane' - so you'd be hard pushed to find somewhere 'useful'. Not only that the cpu would have to return from copy_from_user_iter() before correcting the mis-speculation. I can't imagine that happening - even without all the 'return thunk' stuff. The same might be true for copy_from_user(). It might only be get_user() that actually has any chance of being exploited. > > And at some point this access_ok() didn't even exist, because we check > the addresses at iter creation time. So this one might be a "belt and > suspenders" check, rather than something critical. IIRC there was a patch to move the access_ok() much nearer the use copy. But it didn't go as far as removing the one from import_iovec(). Although removing that one might make sense. (I've also looked about whether the 'direction' is needed in the 'iter'. 98% of the code knows what it should be - and may contain pointless checks, but some bits seem to rely on it.) David > > (Although I also suspect that when we added ITER_UBUF we might have > created cases where those user addresses aren't checked at iter > creation time any more). > > Linus
© 2016 - 2025 Red Hat, Inc.