From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: Don't unlock vnode(9) in fifo_read() and fifo_write()
To: Vitaliy Makkoveev <mvs@openbsd.org>, tech@openbsd.org
Date: Thu, 18 Apr 2024 14:32:14 +0200

On Thu, Apr 18, 2024 at 01:07:12PM +0200, Martin Pieuchot wrote:
> On 18/04/24(Thu) 11:52, Claudio Jeker wrote:
> > On Thu, Apr 18, 2024 at 11:42:09AM +0200, Martin Pieuchot wrote:
> > > On 17/04/24(Wed) 18:33, Vitaliy Makkoveev wrote:
> > > > On Tue, Apr 16, 2024 at 04:30:08PM +0200, Claudio Jeker wrote:
> > > > > On Tue, Apr 16, 2024 at 04:15:05PM +0300, Vitaliy Makkoveev wrote:
> > > > > > I suspect syzkaller reported panics [1] and [2] happened because sockets
> > > > > > were closed due vnode(9) lock release.
> > > 
> > > How did you come to this conclusion?
> > > 
> > > The bug is a NULL dereference:
> > > 
> > > uvm_fault(0xfffffd806d2c9780, 0x8, 0, 1) -> e
> > > kernel: page fault trap, code=0
> > > Stopped at      fifo_write+0x4e:        movq    0x8(%rax),%r12
> > > 
> > > I don't have the same assembly here, but the offset 0x8 seems to correspond
> > > the `a_uio' field.   So the argument passed to fifo_write() is NULL.
> > > 
> > > How can it be NULL?
> > 
> > As I mentioned I think the problem is that the vnode got recycled and UFS is
> > using a now bad vnode for a totally different operation. This is why nothing
> > makes sense.
> 
> How can it be recycled in the middle of ufs_synlink()?  The vnode should
> have a valid reference since vput() is called after vn_rdwr().  So
> unlocking the vnode shouldn't be a problem there.  I don't understand
> what's happening here.  Somebody wants to look at the reproducer?

Code does more or less:

const char name1 = "./file0aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/file0";
const char name2 = "./file0aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";

mknod("./file0", O_RDONLY, 0); /* 0x1fbd = 017675 -> S_IFIFO is set */
r = open(".", O_RDONLY, 0);
if (r == -1)
	exit(1);
symlinkat(name1, r, name2);

Note name1 is name2 + '/file0' so the symlink tries to link:
./file0aa/file0 to ./file0aa (which works but is a recursive symlink so
you can't open it).

While the code makes a FIFO, nothing uses said fifo (there is no open call
for the fifo).

For me this really smells like a bad UFS bug.
-- 
:wq Claudio