Re: CVS commit: src/sys/kern

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 08/03/2020 à 21:41, Andrew Doran a écrit :
> 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.

No, it is not. KMEM_SIZE has found real bugs. Now this useful feature has
been lessened just to shut a sanitizer which was pointing another issue.

Again, as said previously, the real bug here is that you are making a
caller assume specific alignment from an allocator that _does not_
guarantee this alignment. To fix that, either (1) revert the assumption or
(2) make it part of the allocator's contract to guarantee this alignment.

I don't have a preference on (1) or (2), but the current design is more a
hack than anything else, and the subsequent change in KMEM_SIZE is
definitely wrong.

>> 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.

Not sure what you mean, but KASAN definitely needs to be enabled on
kmem. I've checked the code, and actually it is fine regarding KASAN for
now, because the computation of the redzone doesn't take KMEM_SIZE into
account (and so isn't affected by the fact that the location changed).