Re: [vfs_cache] Re: CVS commit: src/sys/kern
On Sun, Mar 29, 2020 at 11:41:23AM +0200, Maxime Villard wrote:
> Le 23/03/2020 ? 21:02, Andrew Doran a ?crit?:
> > Module Name:src
> > Committed By: ad
> > Date: Mon Mar 23 20:02:14 UTC 2020
> >
> > Modified Files:
> > src/sys/kern: vfs_cache.c
> >
> > Log Message:
> > cache_remove(): remove from the vnode list first, so cache_revlookup()
> > doesn't try to re-activate an entry no longer on the LRU list.
> >
> >
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.133 -r1.134 src/sys/kern/vfs_cache.c
> >
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
>
> It appears that your recent changes in vfs_cache.c have introduced a
> use-after-free. Booting KASAN gives:
>
> ASan: Unauthorized Access In 0x...: Addr 0x... [1 byte, read,
> PoolUseAfterFree]
>
> It seems that the problem is here:
>
> 557 if (nameiop == CREATE && (cnflags & ISLASTCN) != 0) {
> 558 /*
> 559* Last component and we are preparing to create
> 560* the named object, so flush the negative cache
> 561* entry.
> 562*/
> 563 COUNT(ncs_badhits);
> 564 cache_remove(ncp, true);< HERE
> 565 hit = false;
> 566 } else {
> 567 COUNT(ncs_neghits);
> 568 SDT_PROBE(vfs, namecache, lookup, hit, dvp, name,
> 569 namelen, 0, 0);
> 570 /* found neg entry; vn is already null from above */
> 571 hit = true;
> 572 }
> 573 if (iswht_ret != NULL) {
> 574 /*
> 575* Restore the ISWHITEOUT flag saved earlier.
> 576*/
> 577 *iswht_ret = ncp->nc_whiteout; <-- ouch
> 578 } else {
> 579 KASSERT(!ncp->nc_whiteout); <-- ouch
> 580 }
>
> cache_remove() frees 'ncp', and then 'ncp->nc_whiteout' is read.
Fixed with vfs_cache.c 1.136. Thank you for bringing it to my attention.
Andrew
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Apr 05, 2020 at 05:26:47PM +, Jaromir Dolecek wrote: > Module Name: src > Committed By: jdolecek > Date: Sun Apr 5 17:26:47 UTC 2020 > > Modified Files: > src/sys/arch/xen/xen: if_xennet_xenbus.c xennetback_xenbus.c > > Log Message: > remove support for legacy rx-flip mode for xennet(4)/xvif(4), making > rx-copy (first shipped in NetBSD 6.0 in 2012) the only supported > mode did you actually read my reply to your proposal to port-xen ? The example provided in the xen 4.11 sources still use rx-flip, so I'm not sure it's not supported any more by linux dom0. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/lib/libc/string
Date:Sat, 4 Apr 2020 21:16:45 +
From:David Holland
Message-ID: <20200404211645.ga19...@netbsd.org>
| I fail to see any scenario in which it's legitimate for an application
| to scribble in internal data belonging to libc. Why should this be
| permitted?
You've never done something like
p = getenv("PATH"); /* NULL check omitted here */
while (q = strchr(p, ':')) {
*q = '\0';
/* use p to do a lookup, printf, or whatever */
*q = ':';
p = q + 1;
}
/* use p here too */
??
If you have, what's the difference, genenv() is also returning data
from libc, it could also be const char *, but isn't.
In neither case is this what would normally be called "internal data"
though, it isn't as if we're directly modifying the data structs malloc()
uses (that would be internal data) - these are the results returned from
libc to the application.
| I don't understand. It is a bug that the library returns a writeable
| pointer to data that should not be modified.
But we aren't returning a pointer to data that can't be modified,
libc doesn't care, one way or the other. Apps shouldn't modify it
(shouldn't attempt to) because of portability concerns with other
implementations.If we did as Joerg suggested and mmapped
the message catalog pages for the relevant locale, and simply returned
a pointer to the relevant entry, one of those could be us.But isn't.
| Anyway, this is moot because strerror is defined by C;
That was my point. The entry in the BUGS section of the man page was
merely a rant, as this isn't (wasn't ever) something that can be fixed.
| but because it *should* be const,
| it should be (and is) declared with __aconst in string.h.
|
| Please don't change that.
Not planning anything like that - this was all about the man page.
| Also, perhaps we should swipe the text about not modifyingthe string
| buffer from strsignal.3.
I believe that what is in strerror(3) now is more or less aligned
with the intent (if not the actual language) of that.
Note that strsignal(3) doesn't contain a BUGS section ranting about
how it should really be const char *strsignal().
kre
