Messages in this thread | | | From | Peter Collingbourne <> | Date | Thu, 9 Dec 2021 20:02:19 -0800 | Subject | Re: [PATCH v3 3/6] fs: use copy_from_user_nolog() to copy mount() data |
| |
On Thu, Dec 9, 2021 at 6:59 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Thu, 9 Dec 2021 at 22:42, Peter Collingbourne <pcc@google.com> wrote: > > > > With uaccess logging the contract is that the kernel must not report > > > > accessing more data than necessary, as this can lead to false positive > > > > reports in downstream consumers. This generally works out of the box > > > > when instrumenting copy_{from,to}_user(), but with the data argument > > > > to mount() we use copy_from_user() to copy PAGE_SIZE bytes (or as > > > > much as we can, if the PAGE_SIZE sized access failed) and figure out > > > > later how much we actually need. > > > > > > > > To prevent this from leading to a false positive report, use > > > > copy_from_user_nolog(), which will prevent the access from being logged. > > > > Recall that it is valid for the kernel to report accessing less > > > > data than it actually accessed, as uaccess logging is a best-effort > > > > mechanism for reporting uaccesses. > > > > > > > > Link: https://linux-review.googlesource.com/id/I5629b92a725c817acd9a861288338dd605cafee6 > > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > > > --- > > > > fs/namespace.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/namespace.c b/fs/namespace.c > > > > index 659a8f39c61a..8f5f2aaca64e 100644 > > > > --- a/fs/namespace.c > > > > +++ b/fs/namespace.c > > > > @@ -31,6 +31,7 @@ > > > > #include <uapi/linux/mount.h> > > > > #include <linux/fs_context.h> > > > > #include <linux/shmem_fs.h> > > > > +#include <linux/uaccess-buffer.h> > > > > > > > > #include "pnode.h" > > > > #include "internal.h" > > > > @@ -3197,7 +3198,12 @@ static void *copy_mount_options(const void __user * data) > > > > if (!copy) > > > > return ERR_PTR(-ENOMEM); > > > > > > > > - left = copy_from_user(copy, data, PAGE_SIZE); > > > > + /* > > > > + * Use copy_from_user_nolog to avoid reporting overly large accesses in > > > > + * the uaccess buffer, as this can lead to false positive reports in > > > > + * downstream consumers. > > > > + */ > > > > + left = copy_from_user_nolog(copy, data, PAGE_SIZE); > > > > > > A late idea... > > > Maybe it's better to log them with a new UACCESS_BUFFER_FLAG_OVERREAD > > > flag. Better for user-space, at least can detect UAFs by checking the > > > first byte. And a more logical kernel annotation (maybe will be used > > > in some other tools? or if we ever check user tags in the kernel). > > > > > > Probably not too important today since we use this only in 2 places, > > > but longer term may be better. > > > > I'm not sure about this. The overreads are basically an implementation > > detail of the kernel, so I'm not sure it makes sense to expose them. A > > scheme where we expose all overreads wouldn't necessarily help with > > UAF, because what if for example the kernel reads *behind* the > > user-provided pointer? I guess it could lead to false positives. > > If user-space uses logging to check addressability, then it can safely > check only the first byte (right? there must be at least 1 byte passed > by user-space at that address). And that's enough to detect UAFs.
I was thinking more e.g. what if the kernel reads an entire page with copy_from_user() and takes a subset of it later. Then the first byte could point to some other random allocation in the same page and lead to a false UAF report if we just consider the first byte.
So I think the use cases for accesses with this flag set may be limited to things like fuzzers.
> > > Btw, what's the story with BPF accesses? Can we log them theoretically? > > > > > > Previously the comment said: > > > > > > + /* > > > + * Avoid copy_from_user() here as it may leak information about the BPF > > > + * program to userspace via the uaccess buffer. > > > + */ > > > > > > but now it says something very generic: > > > > > > /* > > > * Avoid logging uaccesses here as the BPF program may not be following > > > * the uaccess log rules. > > > */ > > > > Yes we should be able to log them theoretically, but we don't need to > > do that now. See my reply here: > > > > https://lore.kernel.org/all/CAMn1gO5B5Q3hfN6kugv2wmdFGNhJb75iRX1zmCkw3wnueN1dtg@mail.gmail.com/#:~:text=This%20comment%20was,the%20comment%20accordingly. > > I see. These could be marked with another flag. > I don't have a strong opinion about this. But I am mentioning this > because my experience is that it's better to expose more raw info from > kernel in these cases, rather than hardcoding policies into kernel > code (what's ignored/why/when) b/c a delay from another kernel change > to wide deployment is 5+ years and user-space code may need to detect > and deal with all various versions of the kernel logic. > Say, fuzzing may still want to know about the mount options (rather > than no signal that the kernel reads at least something at that > address). But adding them later with a flag is not really a backwards > compatible change b/c you now have addressability checking code that's > not checking the new flag and will produce false positives.
I think this is a good point. I'll see about adding flags for the BPF and overread cases.
Peter
| |