Page MenuHomeFreeBSD

lindebugfs: Pass user buffer pointers to the read/write file operations
ClosedPublic

Authored by jhb on Thu, Mar 12, 8:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 20, 2:35 AM
Unknown Object (File)
Thu, Mar 19, 11:39 PM
Unknown Object (File)
Thu, Mar 19, 5:35 PM
Unknown Object (File)
Thu, Mar 19, 5:32 PM
Unknown Object (File)
Thu, Mar 19, 1:38 PM
Unknown Object (File)
Thu, Mar 19, 11:14 AM
Unknown Object (File)
Thu, Mar 19, 8:27 AM
Unknown Object (File)
Thu, Mar 19, 8:27 AM
Subscribers

Details

Summary

The Linux file_operations API expects the read and write operations
to take a single user buffer pointer (along with the length and the
file offset as an in/out parameter).

However, the debugfs_fill function was violating this part of the
contract as it was passing down kernel pointers instead. An earlier
commit (5668c22a13c6befa9b8486387d38457c40ce7af4) hacked around this
by modifying simple_read_from_buffer() to treat its user pointer
argument as a kernel pointer instead. However, other commits keep
tripping over this same API mismatch
(e.g. 78e25e65bf381303c8bdac9a713ab7b26a854b8c passes a kernel pointer
to copy_from_user in fops_str_write).

Instead, change debugfs_fill to use the "raw" pseudofs mode where the
uio is passed down to directly to the fill callback rather than an
sbuf. debufs_fill now iterates over the iovec in the uio similar to
the implementation of uiomove invoking the read or write operation on
each user pointer.

This also fixes a tiny bug where the initial file offset from
uio_offset was ignored. Instead, the operations were always invoked
with a file offset of 0.

As part of this, revert the the changes to simple_read_from_buffer()
from commit 5668c22a13c6befa9b8486387d38457c40ce7af4.

Also as part of this, the simple_attr_read/write methods and seq_read
now also need to accept and handle user pointers (also matching the
API in Linux).

For simple_attr_write*(), copy the user buffer into a kernel buffer
before parsing. Also, do not permit writes at an offset as it's
unclear what the semantics for those would even be (perhaps you would
write out the formatted value into a buffer first and then allow the
copy_from_user to overwrite/extend that buffer and then re-parse the
integer value?). The old handling of *ppos for writes was definitely
wrong before and only worked for an offset of 0 anyway.

Sponsored by: AFRL, DARPA

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71373
Build 68256: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Thu, Mar 12, 8:50 PM

This would be an alternate fix instead of D55816. Note that I have compiled this, but have no good way to test it so would appreciate some testing.

I should also mention in the commit log that this fixes a bug where the initial value of uio_offset was ignored. (So if you did a pread() with a non-zero offset of a debugfs node you would get the wrong data, but that's probably a case that never occurs in practice.)

This also makes me appreciate struct uio.

I need to go ans see what all is affected by this to see what I have avail to test in wifi land. I'll try tomorrow; I don't think I'll still make it tonight.

Thanks a lot for going the extra mile!

I found another place I need to update (simple_attr), will update the review later today with those changes as well.

jhb added a reviewer: jfree.
sys/compat/linuxkpi/common/src/linux_simple_attr.c
122 ↗(On Diff #173652)

Using simple_read_from_buffer() here I could pull out into a separate commit before the rest of this change if that is helpful with bisecting, etc.

132 ↗(On Diff #173652)

I think the += 1 here is actually not correct in the existing code as we already include the nul terminator above when setting the initial value of ret.

bz requested changes to this revision.EditedMon, Mar 16, 12:34 AM

I managed to test and my memory was correct though I couldn't find it Friday;
there's more sbufs out there which now need to be changes I assume, e.g., sys/compat/linuxkpi/common/src/linux_seq_file.c

They come from simple_open() in the drivers directly often and not just the macros.

If you follow bb_18 for example in rtw88 you'll end up in the below:

seq_read():
        m = f->private_data;
        sbuf = m->buf;
...
--- trap 0xc, rip = 0xffffffff81097cba, rsp = 0xfffffe0067a15850, rbp = 0xfffffe0067a15850 ---
memcpy_std() at memcpy_std+0x10a/frame 0xfffffe0067a15850
seq_read() at seq_read+0x9c/frame 0xfffffe0067a15890
debugfs_fill() at debugfs_fill+0x13e/frame 0xfffffe0067a15ba0
pfs_read() at pfs_read+0x29d/frame 0xfffffe0067a15c00
...
This revision now requires changes to proceed.Mon, Mar 16, 12:34 AM

Ok, so with seq_file fixed, I am now able to build all the debugfs bits in the wireless drivers under CHERI, so I think this finds all of them. I've split out the changes to use simple_read_from_buffer into a separate commit before this one now.

In D55833#1278447, @jhb wrote:

Ok, so with seq_file fixed, I am now able to build all the debugfs bits in the wireless drivers under CHERI, so I think this finds all of them. I've split out the changes to use simple_read_from_buffer into a separate commit before this one now.

Well, all but rtw88 have debugfs disabled in main and stable/15. I should go and see what it takes to re-enable the other ones as well.

In D55833#1278515, @bz wrote:
In D55833#1278447, @jhb wrote:

Ok, so with seq_file fixed, I am now able to build all the debugfs bits in the wireless drivers under CHERI, so I think this finds all of them. I've split out the changes to use simple_read_from_buffer into a separate commit before this one now.

Well, all but rtw88 have debugfs disabled in main and stable/15. I should go and see what it takes to re-enable the other ones as well.

On the older CheriBSD (currently merged up through May 2, 2025 of FreeBSD main), iwlwifi was also enabled at least. For CHERI I have to go add __capability annotations on all the user pointers and I had to do that in iwlwifi, not just rtw88. However, that was all I had to do, it was already assuming user pointers in the helpers iwlwifi uses for debugfs nodes.

bz requested changes to this revision.Tue, Mar 17, 12:13 AM

Not sure if this should be applied to the old style first as well and then this one on top. Either way it needs to be fixed to avoid the -EFAULT.

sys/compat/linuxkpi/common/include/linux/fs.h
371

Another (unrelated) one wich may have worked before by accident:

This check is bogus. buf_remain is size_t and cannot be < 0.

(a) check *ppos >= 0
(b) if (read_size == 0 || (*ppos - buf_size) <= 0) return (0) as otherwise the copy_to_user() later will have a read_size of 0 which will work well, but num_read will be 0 and we return -EFAULT.

Sorry I also moved initialization after checking values for good behaviour, which makes the diff look weirder than it is.

-       void *read_pos = ((char *) orig) + *ppos;
-       size_t buf_remain = buf_size - *ppos;
+       void *read_pos;
+       size_t buf_remain;
        ssize_t num_read;

-       if (buf_remain < 0 || buf_remain > buf_size)
+       if (*ppos < 0)
                return (-EINVAL);
+       if (read_size == 0 || (*ppos - buf_size) <= 0)
+               return (0);
 
+       buf_remain = buf_size - *ppos;
        if (read_size > buf_remain)
                read_size = buf_remain;
 
+       read_pos = ((char *) orig) + *ppos;
        /* copy_to_user returns number of bytes NOT read */
...
This revision now requires changes to proceed.Tue, Mar 17, 12:13 AM

Ok, with my above change and the sbuf_clear() from the other review and your two changes I believe dumping all readable entries from rtw88 works.
I have not yet tried the write path.

The simple read/write/read check seems to work:

# cat coex_enable
coex mechanism enabled
# echo 0 >> coex_enable
# cat coex_enable
coex mechanism disabled

For more write testing I think I'll let time tell :)

In D55833#1278626, @bz wrote:

Not sure if this should be applied to the old style first as well and then this one on top. Either way it needs to be fixed to avoid the -EFAULT.

Isn't this the fix from D55845 you already approved? (It's a followup to this commit in this stack/series)

Maybe get D55845 in before this one?

sys/compat/linuxkpi/common/include/linux/fs.h
371

Disregard; apart from the *ppos check this is indeed D55845 and I have a local bug with <= which should be >=

This revision is now accepted and ready to land.Tue, Mar 17, 7:01 PM