Re: CVS commit: src/sys/kern

[Andrew Doran <ad%netbsd.org@localhost>]

On Sun, Mar 08, 2020 at 08:34:29AM +0100, Maxime Villard wrote:
> Le 08/03/2020 ? 01:31, Andrew Doran a ?crit?:
> > Module Name:	src
> > Committed By:	ad
> > Date:		Sun Mar  8 00:31:19 UTC 2020
> > 
> > Modified Files:
> > 	src/sys/kern: subr_kmem.c
> > 
> > Log Message:
> > KMEM_SIZE: append the size_t to the allocated buffer, rather than
> > prepending, so it doesn't screw up the alignment of the buffer.
> > 
> > Reported-by: syzbot+c024c50570cccac51532%syzkaller.appspotmail.com@localhost
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.78 -r1.79 src/sys/kern/subr_kmem.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> This is wrong. The point of KMEM_SIZE is to store at a reliable location
> the size of the buffer, in order to then verify that the 'size' argument
> given to kmem_free() is correct.
> 
> Here, you are using that size argument to compute the location, which
> breaks the whole point of the check.

Sure I understand the frustration, but it's still correct according to
the parameters I set for it 10+ years ago, which were for it to be a
quick-n-dirty diagnostic aid.

> Also it probably collides with the KASAN shadow.

Hmm, is that purely an alignment issue then, because it works in 8 byte
cells?  Otherwise it sounds like this should not be enabled with KASAN at
all.

Andrew
 
> Please revert this change.
>
> As said previously, if cacheline alignment is expected this way, then it
> should clearly be part of the kmem contract, and documented to be so.