Date: Fri, 12 Jan 2024 17:29:41 -0500 From: Mark Johnston <markj@freebsd.org> To: Ravi Pokala <rpokala@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: d07acc58d898 - main - systm: Relax __result_use_check annotations Message-ID: <ZaG9VQqG06ejJfia@nuc> In-Reply-To: <BB320FF2-6B2B-491C-903B-4D8C1F0514C4@panasas.com> References: <202401122112.40CLC7nM039270@gitrepo.freebsd.org> <BB320FF2-6B2B-491C-903B-4D8C1F0514C4@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jan 12, 2024 at 01:25:43PM -0800, Ravi Pokala wrote: > Hi Mark, > > > failing to check for errors when copying *in* is a much more severe bug > > In that you might get panics. How do you mean? > But failing to check for errors when copying *out* can be a security leak; depending on the context, a panic might be preferable. If something in the kernel fails to check for copyout() errors, userspace will potentially be confused since it expected the kernel to overwrite some memory when it in fact did not. Failing to check for copyin() errors can cause the kernel to potentially end up treating uninitialized (kernel) memory as initialized. That can have security ramifications depending on what the kernel does after. > I agree with what Mateusz just said: make __result_use_check contingent on `clang'. I don't want to do exactly that. Here is my attempt: https://reviews.freebsd.org/D43426 > Thanks, > > Ravi (rpokala@) > > -----Original Message----- > From: <owner-src-committers@freebsd.org <mailto:owner-src-committers@freebsd.org>> on behalf of Mark Johnston <markj@FreeBSD.org <mailto:markj@FreeBSD.org>> > Date: Friday, January 12, 2024 at 13:12 > To: <src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org>>, <dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org>>, <dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org>> > Subject: git: d07acc58d898 - main - systm: Relax __result_use_check annotations > > > The branch main has been updated by markj: > > > URL: https://cgit.FreeBSD.org/src/commit/?id=d07acc58d8987e8e1205f4a82b77e847ea2d60d3 <https://cgit.FreeBSD.org/src/commit/?id=d07acc58d8987e8e1205f4a82b77e847ea2d60d3>; > > > commit d07acc58d8987e8e1205f4a82b77e847ea2d60d3 > Author: Mark Johnston <markj@FreeBSD.org <mailto:markj@FreeBSD.org>> > AuthorDate: 2024-01-12 15:07:28 +0000 > Commit: Mark Johnston <markj@FreeBSD.org <mailto:markj@FreeBSD.org>> > CommitDate: 2024-01-12 20:56:00 +0000 > > > systm: Relax __result_use_check annotations > > > When compiling with gcc, functions annotated this way can not have their > return values cast away, e.g., with `(void)copyout(...)`. clang permits > it but gcc does not. Since we have a number of such casts for calls > which copy data out of the kernel, and since failing to check for errors > when copying *in* is a much more severe bug, remove some of the > annotations in order to make the gcc build happy. > > > Reviewed by: kib > Reported by: Jenkins > Fixes: 8e36732e6eb5 ("systm: Annotate copyin() and related functions with __result_use_check") > Differential Revision: https://reviews.freebsd.org/D43418 <https://reviews.freebsd.org/D43418>; > --- > sys/sys/systm.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > > diff --git a/sys/sys/systm.h b/sys/sys/systm.h > index 2da177af91f0..508690cd639e 100644 > --- a/sys/sys/systm.h > +++ b/sys/sys/systm.h > @@ -289,9 +289,9 @@ int __result_use_check copyin(const void * __restrict udaddr, > void * _Nonnull __restrict kaddr, size_t len); > int __result_use_check copyin_nofault(const void * __restrict udaddr, > void * _Nonnull __restrict kaddr, size_t len); > -int __result_use_check copyout(const void * _Nonnull __restrict kaddr, > +int copyout(const void * _Nonnull __restrict kaddr, > void * __restrict udaddr, size_t len); > -int __result_use_check copyout_nofault(const void * _Nonnull __restrict kaddr, > +int copyout_nofault(const void * _Nonnull __restrict kaddr, > void * __restrict udaddr, size_t len); > > > #ifdef SAN_NEEDS_INTERCEPTORS > @@ -313,11 +313,11 @@ int64_t fuword64(volatile const void *base); > int __result_use_check fueword(volatile const void *base, long *val); > int __result_use_check fueword32(volatile const void *base, int32_t *val); > int __result_use_check fueword64(volatile const void *base, int64_t *val); > -int __result_use_check subyte(volatile void *base, int byte); > -int __result_use_check suword(volatile void *base, long word); > -int __result_use_check suword16(volatile void *base, int word); > -int __result_use_check suword32(volatile void *base, int32_t word); > -int __result_use_check suword64(volatile void *base, int64_t word); > +int subyte(volatile void *base, int byte); > +int suword(volatile void *base, long word); > +int suword16(volatile void *base, int word); > +int suword32(volatile void *base, int32_t word); > +int suword64(volatile void *base, int64_t word); > uint32_t casuword32(volatile uint32_t *base, uint32_t oldval, uint32_t newval); > u_long casuword(volatile u_long *p, u_long oldval, u_long newval); > int casueword32(volatile uint32_t *base, uint32_t oldval, uint32_t *oldvalp, > > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?ZaG9VQqG06ejJfia>