Re: [vfs_cache] Re: CVS commit: src/sys/kern

2020-04-05 Thread Andrew Doran
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


[vfs_cache] Re: CVS commit: src/sys/kern

2020-03-29 Thread Maxime Villard
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.

Maxime