Since seq_read_iter looks mature enough to be used for all procfs files,
re-allow applications to perform zero-copy data forwarding from them.
According to the sendfile(2) man-page, it is still enough for the file
being read to support mmap-like operations, and the proc files support
memory mapping, so returning -EINVAL was an inconsistency.
Some executable-inspecting tools rely on patching entry point
instructions with minimal machine code that uses sendfile to read
/proc/self/maps to stdout. The sendfile call allows them to do it
faster and without excessive allocations.
This is inspired by the series by Cristoph Hellwig (linked).
Link: https://lore.kernel.org/lkml/[email protected]/
Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Cc: Alexey Dobriyan <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Arkadiusz Kozdra (Arusekk) <[email protected]>
---
fs/proc/array.c | 9 +++++----
fs/proc/base.c | 36 ++++++++++++++++++++++++------------
fs/proc/fd.c | 3 ++-
fs/proc/proc_net.c | 4 ++--
fs/proc/task_mmu.c | 12 ++++++++----
fs/proc/task_nommu.c | 3 ++-
6 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index bb87e4d89cd8..3c3da655a6d0 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -797,9 +797,10 @@ static int children_seq_open(struct inode *inode, struct file *file)
}
const struct file_operations proc_tid_children_operations = {
- .open = children_seq_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = seq_release,
+ .open = children_seq_open,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
+ .llseek = seq_lseek,
+ .release = seq_release,
};
#endif /* CONFIG_PROC_CHILDREN */
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3851bfcdba56..b9921a0c2d3d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -535,7 +535,8 @@ static ssize_t lstats_write(struct file *file, const char __user *buf,
static const struct file_operations proc_lstats_operations = {
.open = lstats_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = lstats_write,
.llseek = seq_lseek,
.release = single_release,
@@ -784,7 +785,8 @@ static int proc_single_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_single_file_operations = {
.open = proc_single_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = single_release,
};
@@ -1473,7 +1475,8 @@ static int sched_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_pid_sched_operations = {
.open = sched_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = sched_write,
.llseek = seq_lseek,
.release = single_release,
@@ -1548,7 +1551,8 @@ static int sched_autogroup_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_pid_sched_autogroup_operations = {
.open = sched_autogroup_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = sched_autogroup_write,
.llseek = seq_lseek,
.release = single_release,
@@ -1651,7 +1655,8 @@ static int timens_offsets_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_timens_offsets_operations = {
.open = timens_offsets_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = timens_offsets_write,
.llseek = seq_lseek,
.release = single_release,
@@ -1708,7 +1713,8 @@ static int comm_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_pid_set_comm_operations = {
.open = comm_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = comm_write,
.llseek = seq_lseek,
.release = single_release,
@@ -2497,7 +2503,8 @@ static int proc_timers_open(struct inode *inode, struct file *file)
static const struct file_operations proc_timers_operations = {
.open = proc_timers_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = seq_release_private,
};
@@ -2589,7 +2596,8 @@ static int timerslack_ns_open(struct inode *inode, struct file *filp)
static const struct file_operations proc_pid_set_timerslack_ns_operations = {
.open = timerslack_ns_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.write = timerslack_ns_write,
.llseek = seq_lseek,
.release = single_release,
@@ -3041,7 +3049,8 @@ static int proc_projid_map_open(struct inode *inode, struct file *file)
static const struct file_operations proc_uid_map_operations = {
.open = proc_uid_map_open,
.write = proc_uid_map_write,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_id_map_release,
};
@@ -3049,7 +3058,8 @@ static const struct file_operations proc_uid_map_operations = {
static const struct file_operations proc_gid_map_operations = {
.open = proc_gid_map_open,
.write = proc_gid_map_write,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_id_map_release,
};
@@ -3057,7 +3067,8 @@ static const struct file_operations proc_gid_map_operations = {
static const struct file_operations proc_projid_map_operations = {
.open = proc_projid_map_open,
.write = proc_projid_map_write,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_id_map_release,
};
@@ -3108,7 +3119,8 @@ static int proc_setgroups_release(struct inode *inode, struct file *file)
static const struct file_operations proc_setgroups_operations = {
.open = proc_setgroups_open,
.write = proc_setgroups_write,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_setgroups_release,
};
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 07fc4fad2602..9fa169f05548 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -77,7 +77,8 @@ static int seq_fdinfo_open(struct inode *inode, struct file *file)
static const struct file_operations proc_fdinfo_file_operations = {
.open = seq_fdinfo_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = single_release,
};
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index 15c2e55d2ed2..a223480189f9 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -76,7 +76,7 @@ static int seq_release_net(struct inode *ino, struct file *f)
static const struct proc_ops proc_net_seq_ops = {
.proc_open = seq_open_net,
- .proc_read = seq_read,
+ .proc_read_iter = seq_read_iter,
.proc_write = proc_simple_write,
.proc_lseek = seq_lseek,
.proc_release = seq_release_net,
@@ -188,7 +188,7 @@ static int single_release_net(struct inode *ino, struct file *f)
static const struct proc_ops proc_net_single_ops = {
.proc_open = single_open_net,
- .proc_read = seq_read,
+ .proc_read_iter = seq_read_iter,
.proc_write = proc_simple_write,
.proc_lseek = seq_lseek,
.proc_release = single_release_net,
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e862cab69583..7655f5018945 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -351,7 +351,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_map_release,
};
@@ -1009,14 +1010,16 @@ static int smaps_rollup_release(struct inode *inode, struct file *file)
const struct file_operations proc_pid_smaps_operations = {
.open = pid_smaps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_map_release,
};
const struct file_operations proc_pid_smaps_rollup_operations = {
.open = smaps_rollup_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = smaps_rollup_release,
};
@@ -1947,7 +1950,8 @@ static int pid_numa_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_numa_maps_operations = {
.open = pid_numa_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_map_release,
};
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index a6d21fc0033c..d17f929ad8a7 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -295,7 +295,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = map_release,
};
--
2.31.1
On Tue, Apr 27, 2021 at 08:34:15PM +0200, Arkadiusz Kozdra (Arusekk) wrote:
> Since seq_read_iter looks mature enough to be used for all procfs files,
> re-allow applications to perform zero-copy data forwarding from them.
> According to the sendfile(2) man-page, it is still enough for the file
> being read to support mmap-like operations, and the proc files support
> memory mapping, so returning -EINVAL was an inconsistency.
Linus did object to blindly switching over all instances.
> Some executable-inspecting tools rely on patching entry point
> instructions with minimal machine code that uses sendfile to read
> /proc/self/maps to stdout. The sendfile call allows them to do it
> faster and without excessive allocations.
Patching what entry point?
On Wed, Apr 28, 2021 at 08:12:59 CEST, Christoph Hellwig wrote:
> Patching what entry point?
The instructions at the entry point of the executable being inspected.
The flow of the tool:
- parse ELF headers of the binary to be inspected,
- locate its entry point position in the file,
- write short code at the location (this short code has used sendfile so far),
- execute the patched binary,
- parse the output and extract information about the relevant mappings.
This can be seen as equivalent to setting LD_TRACE_LOADED_OBJECTS,
but also works for static binaries, and is a bit safer.
The problem was reported at:
https://github.com/Gallopsled/pwntools/issues/1871
> Linus did object to blindly switching over all instances.
I know, I read that, but I thought that pointing a real use case, combined
with the new interface being used all throughout the other code, might be
convincing.
I would be happy with only changing the f_ops of /proc/.../maps, even if only
on MMU-enabled systems, but I thought that consistence would be better.
This is my first time contributing to Linux, so I am very sorry for any wrong
assumptions, and glad to learn more.
--
Arusekk
On Wed, Apr 28, 2021 at 03:02:13PM +0200, Arusekk wrote:
> The instructions at the entry point of the executable being inspected.
> The flow of the tool:
> - parse ELF headers of the binary to be inspected,
> - locate its entry point position in the file,
> - write short code at the location (this short code has used sendfile so far),
> - execute the patched binary,
> - parse the output and extract information about the relevant mappings.
> This can be seen as equivalent to setting LD_TRACE_LOADED_OBJECTS,
> but also works for static binaries, and is a bit safer.
>
> The problem was reported at:
> https://github.com/Gallopsled/pwntools/issues/1871
Oh, this patches just the userspace binarz, ok.
> > Linus did object to blindly switching over all instances.
>
> I know, I read that, but I thought that pointing a real use case, combined
> with the new interface being used all throughout the other code, might be
> convincing.
> I would be happy with only changing the f_ops of /proc/.../maps, even if only
> on MMU-enabled systems, but I thought that consistence would be better.
> This is my first time contributing to Linux, so I am very sorry for any wrong
> assumptions, and glad to learn more.
Unless Linus changed his mind just patching the file you care about for
now seems like the best idea.
On Wed, Apr 28, 2021 at 6:03 AM Christoph Hellwig <[email protected]> wrote:
>
> Unless Linus changed his mind just patching the file you care about for
> now seems like the best idea.
I'm ok with expanding splice() use, but I do want it to be on a
case-by-case basis and with comments about what actually used splice()
in the odd circumstances.
Our splice infrastructure is probably a lot safer than it used to be
now that set_fs() is gone, but splice() on odd files does remain
historically a source of not just bugs, but bugs that were security
issues.
So it's mainly a "once bitten, twice shy" thing for me, which is why
I'm more than happy to extend splice(), but want to do so in a very
careful and controlled - and documented - manner, rather than the old
situation where "pretty much everything can do splice, whether it
actually works or not".
Linus
Since seq_read_iter looks mature enough to be used for /proc/<pid>/maps,
re-allow applications to perform zero-copy data forwarding from it.
Some executable-inspecting tools rely on patching entry point
instructions with minimal machine code that uses sendfile to read
/proc/self/maps to stdout. The sendfile call allows them to do it
faster and without excessive allocations.
This is inspired by the series by Cristoph Hellwig (linked).
Changes since v1:
only change proc_pid_maps_operations
Link: https://lore.kernel.org/lkml/[email protected]/
Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Cc: Alexey Dobriyan <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Arkadiusz Kozdra (Arusekk) <[email protected]>
---
fs/proc/task_mmu.c | 3 ++-
fs/proc/task_nommu.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e862cab69583..06282294ddb8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -351,7 +351,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_map_release,
};
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index a6d21fc0033c..e55e79fd0175 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -295,7 +295,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = map_release,
};
--
2.31.1
On Thu, Apr 29, 2021 at 12:05:08PM +0200, Arkadiusz Kozdra (Arusekk) wrote:
> Since seq_read_iter looks mature enough to be used for /proc/<pid>/maps,
> re-allow applications to perform zero-copy data forwarding from it.
>
> Some executable-inspecting tools rely on patching entry point
> instructions with minimal machine code that uses sendfile to read
> /proc/self/maps to stdout. The sendfile call allows them to do it
> faster and without excessive allocations.
What programs do that today? You might want to list them here.
>
> This is inspired by the series by Cristoph Hellwig (linked).
>
> Changes since v1:
>
> only change proc_pid_maps_operations
This should go below the --- line, like the documentation states.
thanks,
greg k-h
On Thu, Apr 29, 2021 at 3:04 AM Arkadiusz Kozdra (Arusekk)
<[email protected]> wrote:
>
> Since seq_read_iter looks mature enough to be used for /proc/<pid>/maps,
> re-allow applications to perform zero-copy data forwarding from it.
I'd really like to hear what the programs are, and what the
performance difference is.
Because I'm surprised that the advantages of splice would really be
noticeable. I don't _dispute_ it, but I really would like this to be
actually _documented_, not just "Some executable-inspecting tools".
What tools (so that if it causes issues later, we have that
knowledge), and what are the performance numbers?
Linus
Since seq_read_iter looks mature enough to be used for /proc/<pid>/maps,
re-allow applications to perform zero-copy data forwarding from it.
Some executable-inspecting tools (e.g. pwntools) rely on patching entry
point instructions with minimal machine code that uses sendfile to read
/proc/self/maps to stdout. The sendfile call allows them to do it
without excessive allocations, which would change the mappings, and
therefore distort the information.
This is inspired by the series by Cristoph Hellwig (linked).
Link: https://lore.kernel.org/lkml/[email protected]/
Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Cc: Alexey Dobriyan <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Arkadiusz Kozdra (Arusekk) <[email protected]>
---
v3:
- Only commit message changed.
- Clarify what tools use this.
- Do not mention performance.
The average execution time of a patched static ELF outputting to a pipe
(the use case of pwntools inspecting mappings of an executable)
was varying both before and after ca. 3.43ms +-0.05ms (I decided that
the performance impact is not worth mentioning in the commit message).
I think that the change should probably marginally improve speed, but
it will most likely also affect the memory footprint and as such likely
minimally decrease power consumption (I suppose it would only be
measurable when the mappings description grows many pages long).
Speed might be more affected in pathological cases like a close-to-OOM
scenario, but I was unable to test this reliably.
I did my tests under qemu-system-x86_64 on a Ryzen 4500 host without
kvm, with default kernel config.
If someone wants to also test this feature of pwntools for themselves,
it can be used as follows and should print something other than None:
$ pip install pwntools
$ python3
>>> from pwn import *
>>> print(ELF("/bin/true").libc)
Sorry for the delay, but it took me much time to figure out some
low-overhead timing methods.
Does this change need selftests? It looks like it should never break
again if it only uses common code hopefully tested elsewhere.
fs/proc/task_mmu.c | 3 ++-
fs/proc/task_nommu.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e862cab69583..06282294ddb8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -351,7 +351,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = proc_map_release,
};
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index a6d21fc0033c..e55e79fd0175 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -295,7 +295,8 @@ static int pid_maps_open(struct inode *inode, struct file *file)
const struct file_operations proc_pid_maps_operations = {
.open = pid_maps_open,
- .read = seq_read,
+ .read_iter = seq_read_iter,
+ .splice_read = generic_file_splice_read,
.llseek = seq_lseek,
.release = map_release,
};
--
2.31.1
On Tue, May 4, 2021 at 4:56 AM Arkadiusz Kozdra (Arusekk)
<[email protected]> wrote:
>
> Some executable-inspecting tools (e.g. pwntools) rely on patching entry
> point instructions with minimal machine code that uses sendfile to read
> /proc/self/maps to stdout. The sendfile call allows them to do it
> without excessive allocations, which would change the mappings, and
> therefore distort the information.
So this was kind of what I was expecting.
The only reason to do this is basically for nefarious purposes, and
it's one of the reasons I didn't feel like doing splice() on
everything should be encouraged.
Yes, yes, I'm sure pwntools can be used by white hats, but honestly,
that seems to be an almost secondary purpose.
Why should the kernel _encourage_ and make it easy to do things like
this? What are the actual advantages for us to do this?
Linus
On Tue, May 4, 2021 at 1:21 PM Arusekk <[email protected]> wrote:
>
> Keeping it the way it is for the sake of security of userspace applications
> looks more like security through obscurity to me.
No, it's simply "no valid use" and "why expose interfaces that don't
need to be exposed".
splice() _has_ been a security issue before. It's why I want to limit
it now. I want to enable it only for cases that seem to be worth
enabling for.
Have we fixed all the splice security issues? I certainly hope so. Are
you correct in stating that there are probably other places that might
be more interesting to attackers? Sure. But none of that changes the
basic issue: why expose this?
Linus
We wtorek, 4 maja 2021 18:01:33 CEST Linus Torvalds pisze:
> it's one of the reasons I didn't feel like doing splice() on
> everything should be encouraged.
This is why I agreed to change only /proc/*/maps.
> The only reason to do this is basically for nefarious purposes,
No real exploit I can think of needs to send this specific file, there are
better candidates in procfs already. It only helps inspecting memory mappings
off-line more reliably than LD_TRACE_LOADED_OBJECTS=1 in as little bytes as
possible.
Or maybe should it be discouraged on regular ext4 files as well?
> Yes, yes, I'm sure pwntools can be used by white hats, but honestly,
> that seems to be an almost secondary purpose.
You got me. But honestly, I disagree, I have already seen pwntools used by
malware analysts and python fans.
> Why should the kernel _encourage_ and make it easy to do things like
> this? What are the actual advantages for us to do this?
Keeping it the way it is for the sake of security of userspace applications
looks more like security through obscurity to me.
I still agree that it may increase attack surface of the kernel. I did not
try racing misaligned splices, nor mmapping files with strange names.
Pwntools need to be adapted anyway in order to support 5.10-5.13 kernels;
I just thought that this is a kind of a regression in the kernel, so I felt
obliged to contribute a fix here, especially after I realized how simple the fix
was.
I heard that patches and their descriptions must really be top quality, but I
was not prepared for justifying the change with anything more than 'it used to
work, but is broken now, this patch brings it back'.
--
Arusekk