Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

/proc/sys/kernel/spl/hostid broken on recent (5.7+) kernels #11878

Closed
nabijaczleweli opened this issue Apr 10, 2021 · 0 comments
Closed

/proc/sys/kernel/spl/hostid broken on recent (5.7+) kernels #11878

nabijaczleweli opened this issue Apr 10, 2021 · 0 comments
Labels
Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Apr 10, 2021 •
edited
Loading

System information

Type Version/Name
Distribution Name Debian
Distribution Version sid, mostly up-to-date as of Sun 11 Apr 00:35:52 CEST 2021
Linux Kernel Linux szarotka 5.10.0-5-amd64 #1 SMP Debian 5.10.24-1 (2021-03-19) x86_64 GNU/Linux
Architecture x32
ZFS Version 2.0.3-6

Describe the problem you're observing

All reads on /proc/sys/kernel/spl/hostid return -EFAULT, tracked this back to this mainline commit with a v5.7-rc1-4-g32927393dc1c describe:

commit 32927393dc1ccd60fb2bdc05b9e8e88753761469
Author: Christoph Hellwig <[email protected]>
Date:   Fri Apr 24 08:43:38 2020 +0200

    sysctl: pass kernel pointers to ->proc_handler

    Instead of having all the sysctl handlers deal with user pointers, which
    is rather hairy in terms of the BPF interaction, copy the input to and
    from  userspace in common code.  This also means that the strings are
    always NUL-terminated by the common code, making the API a little bit
    safer.

    As most handler just pass through the data to one of the common handlers
    a lot of the changes are mechnical.

    Signed-off-by: Christoph Hellwig <[email protected]>
    Acked-by: Andrey Ignatov <[email protected]>
    Signed-off-by: Al Viro <[email protected]>

This makes the access_ok() in copy_to_user() in proc_copyout_string() in proc_dohostid() fail, since ubuffer is kernel memory now – proc_dostring() just does a memcpy.

This is the only affected path, since all others delegate to kernel's proc_do*() functions.
@nabijaczleweli nabijaczleweli added Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang) labels Apr 10, 2021
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 10, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
93cc598 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

This strictens write parsing by disallowing writes with extraneous
whitespace at the end, i.e. "012345678\n" is still fine,
but "012345678   \n" isn't anymore

This also alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
@nabijaczleweli nabijaczleweli mentioned this issue Apr 11, 2021
Closed
13 tasks
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 11, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
d43b823 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

This strictens write parsing by disallowing writes with extraneous
whitespace at the end, i.e. "012345678\n" is still fine,
but "012345678   \n" isn't anymore

This also alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 11, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
45c2446 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 11, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
a74b75e 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed, as is "012345678zupsko", &c.

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 12, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
99e5c52 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed, as is "012345678zupsko", &c.

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this issue Apr 12, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
3a40d12 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed, as is "012345678zupsko", &c.

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
behlendorf pushed a commit to behlendorf/zfs that referenced this issue Apr 21, 2021
@nabijaczleweli @behlendorf
linux/spl: base proc_dohostid() on proc_dostring()
e5c4f86 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed, as is "012345678zupsko", &c.

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
Closes openzfs#11879
sempervictus pushed a commit to sempervictus/zfs that referenced this issue May 31, 2021
@nabijaczleweli
linux/spl: base proc_dohostid() on proc_dostring()
f30e0ef 
This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit
32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers
to ->proc_handler") ‒ 5.7-rc1 and up

The access_ok() check in copy_to_user() in proc_copyout_string() would
always fail, so all userspace reads and writes would fail with EINVAL

proc_dostring() strips only the final new-line,
but simple_strtoul() doesn't actually need a back-trimmed string ‒
writing "012345678   \n" is still allowed, as is "012345678zupsko", &c.

This alters what happens when an invalid value is written ‒
previously it'd get set to what-ever simple_strtoul() returned
(probably 0, thereby resetting it to default), now it does nothing

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11878
Closes openzfs#11879
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@nabijaczleweli