linux-kernel - Re: [PATCH] 9p: add missing locking around taking dentry fid list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 23 May 2024 18:27:28 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Christian Schoenebeck <linux_oss@...debyte.com>
Cc: Eric Van Hensbergen <ericvh@...nel.org>,
	Latchesar Ionkov <lucho@...kov.net>, Greg Kurz <groug@...d.org>,
	Jianyong Wu <jianyong.wu@....com>, stable@...r.kernel.org,
	Eric Van Hensbergen <ericvh@...il.com>, v9fs@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] 9p: add missing locking around taking dentry fid list

Christian Schoenebeck wrote on Thu, May 23, 2024 at 10:34:14AM +0200:
> > The comment still works -- if detry->d_fsdata is NULL then
> > hlist_for_each_entry will stop short and not iterate over anything (it
> > won't bug out), so that part is fine in my opinion.
> 
> I meant the opposite: dentry->d_fsdata not being NULL.

I also meant that in the d_fsdata not being NULL branch, if d_fsdata
turns out to be NULL when it is read under lock later.

> In this case v9fs_fid_find() takes a local copy of the list head
> pointer as `h` without taking a lock before.

It doesn't, it takes &dentry->d_fsdata so the address of d_fsdata before
the lock, but that address cannot change here (another thread cannot
change the address of the dentry) ...(continuing below)

> Then v9fs_fid_find() takes the lock to run hlist_for_each_entry(), but at this
> point `h` could already point at garbage.

.. so *h (in practice, head->first in hlist_for_each_entry()) will
properly contain the first node of the list under lock: either NULL if
we just cleared it (at which point the loop won't iterate anything), or
a new list if other items have been added meanwhile.


I really think it's safe, but I do agree that it's hard to read, happy
to move the `h = &dentry->d_fsdata` inside the lock if you prefer -- it
compiles to the same code for me (x86_64/gcc 13.2.0)
-- 
Dominique Martinet | Asmadeus