* [PATCH 0/3] Fix some seq_file users that were recently broken
@ 2021-02-05 0:36 NeilBrown
2021-02-05 0:36 ` [PATCH 3/3] net: fix iteration for sctp transport seq_files NeilBrown
2021-02-05 22:35 ` [PATCH 0/3] Fix some seq_file users that were recently broken Andrew Morton
0 siblings, 2 replies; 8+ messages in thread
From: NeilBrown @ 2021-02-05 0:36 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro, Jonathan Corbet
Cc: Xin Long, linux-kernel, linux-doc, Ingo Molnar, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
A recent change to seq_file broke some users which were using seq_file
in a non-"standard" way ... though the "standard" isn't documented, so
they can be excused. The result is a possible leak - of memory in one
case, of references to a 'transport' in the other.
These three patches:
1/ document and explain the problem
2/ fix the problem user in x86
3/ fix the problem user in net/sctp
I suspect the patches should each go through the relevant subsystems,
but I'm including akpm as the original went through him.
Thanks,
NeilBrown
---
NeilBrown (3):
seq_file: document how per-entry resources are managed.
x86: fix seq_file iteration for pat/memtype.c
net: fix iteration for sctp transport seq_files
Documentation/filesystems/seq_file.rst | 6 ++++++
arch/x86/mm/pat/memtype.c | 4 ++--
net/sctp/proc.c | 16 ++++++++++++----
3 files changed, 20 insertions(+), 6 deletions(-)
--
Signature
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] net: fix iteration for sctp transport seq_files
2021-02-05 0:36 [PATCH 0/3] Fix some seq_file users that were recently broken NeilBrown
@ 2021-02-05 0:36 ` NeilBrown
2021-02-05 14:32 ` Marcelo Ricardo Leitner
2021-02-05 22:35 ` [PATCH 0/3] Fix some seq_file users that were recently broken Andrew Morton
1 sibling, 1 reply; 8+ messages in thread
From: NeilBrown @ 2021-02-05 0:36 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner
Cc: Xin Long, linux-kernel, David S. Miller, linux-sctp, netdev
The sctp transport seq_file iterators take a reference to the transport
in the ->start and ->next functions and releases the reference in the
->show function. The preferred handling for such resources is to
release them in the subsequent ->next or ->stop function call.
Since Commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration
code and interface") there is no guarantee that ->show will be called
after ->next, so this function can now leak references.
So move the sctp_transport_put() call to ->next and ->stop.
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
Reported-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
net/sctp/proc.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index f7da88ae20a5..982a87b3e11f 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -215,6 +215,12 @@ static void sctp_transport_seq_stop(struct seq_file *seq, void *v)
{
struct sctp_ht_iter *iter = seq->private;
+ if (v && v != SEQ_START_TOKEN) {
+ struct sctp_transport *transport = v;
+
+ sctp_transport_put(transport);
+ }
+
sctp_transport_walk_stop(&iter->hti);
}
@@ -222,6 +228,12 @@ static void *sctp_transport_seq_next(struct seq_file *seq, void *v, loff_t *pos)
{
struct sctp_ht_iter *iter = seq->private;
+ if (v && v != SEQ_START_TOKEN) {
+ struct sctp_transport *transport = v;
+
+ sctp_transport_put(transport);
+ }
+
++*pos;
return sctp_transport_get_next(seq_file_net(seq), &iter->hti);
@@ -277,8 +289,6 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
sk->sk_rcvbuf);
seq_printf(seq, "\n");
- sctp_transport_put(transport);
-
return 0;
}
@@ -354,8 +364,6 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
seq_printf(seq, "\n");
}
- sctp_transport_put(transport);
-
return 0;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: fix iteration for sctp transport seq_files
2021-02-05 0:36 ` [PATCH 3/3] net: fix iteration for sctp transport seq_files NeilBrown
@ 2021-02-05 14:32 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 8+ messages in thread
From: Marcelo Ricardo Leitner @ 2021-02-05 14:32 UTC (permalink / raw)
To: NeilBrown
Cc: Andrew Morton, Alexander Viro, Vlad Yasevich, Neil Horman,
Xin Long, linux-kernel, David S. Miller, linux-sctp, netdev
On Fri, Feb 05, 2021 at 11:36:30AM +1100, NeilBrown wrote:
> The sctp transport seq_file iterators take a reference to the transport
> in the ->start and ->next functions and releases the reference in the
> ->show function. The preferred handling for such resources is to
> release them in the subsequent ->next or ->stop function call.
>
> Since Commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration
> code and interface") there is no guarantee that ->show will be called
> after ->next, so this function can now leak references.
>
> So move the sctp_transport_put() call to ->next and ->stop.
>
> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
> Reported-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some seq_file users that were recently broken
2021-02-05 0:36 [PATCH 0/3] Fix some seq_file users that were recently broken NeilBrown
2021-02-05 0:36 ` [PATCH 3/3] net: fix iteration for sctp transport seq_files NeilBrown
@ 2021-02-05 22:35 ` Andrew Morton
2021-02-06 22:29 ` Jakub Kicinski
2021-02-07 22:45 ` NeilBrown
1 sibling, 2 replies; 8+ messages in thread
From: Andrew Morton @ 2021-02-05 22:35 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Jonathan Corbet, Xin Long, linux-kernel,
linux-doc, Ingo Molnar, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
On Fri, 05 Feb 2021 11:36:30 +1100 NeilBrown <neilb@suse.de> wrote:
> A recent change to seq_file broke some users which were using seq_file
> in a non-"standard" way ... though the "standard" isn't documented, so
> they can be excused. The result is a possible leak - of memory in one
> case, of references to a 'transport' in the other.
>
> These three patches:
> 1/ document and explain the problem
> 2/ fix the problem user in x86
> 3/ fix the problem user in net/sctp
>
1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
interface") was August 2018, so I don't think "recent" applies here?
I didn't look closely, but it appears that the sctp procfs file is
world-readable. So we gave unprivileged userspace the ability to leak
kernel memory?
So I'm thinking that we aim for 5.12-rc1 on all three patches with a cc:stable?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some seq_file users that were recently broken
2021-02-05 22:35 ` [PATCH 0/3] Fix some seq_file users that were recently broken Andrew Morton
@ 2021-02-06 22:29 ` Jakub Kicinski
2021-02-07 21:11 ` Andrew Morton
2021-02-07 22:45 ` NeilBrown
1 sibling, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2021-02-06 22:29 UTC (permalink / raw)
To: Andrew Morton
Cc: NeilBrown, Alexander Viro, Jonathan Corbet, Xin Long,
linux-kernel, linux-doc, Ingo Molnar, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
On Fri, 5 Feb 2021 14:35:50 -0800 Andrew Morton wrote:
> On Fri, 05 Feb 2021 11:36:30 +1100 NeilBrown <neilb@suse.de> wrote:
>
> > A recent change to seq_file broke some users which were using seq_file
> > in a non-"standard" way ... though the "standard" isn't documented, so
> > they can be excused. The result is a possible leak - of memory in one
> > case, of references to a 'transport' in the other.
> >
> > These three patches:
> > 1/ document and explain the problem
> > 2/ fix the problem user in x86
> > 3/ fix the problem user in net/sctp
>
> 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
> interface") was August 2018, so I don't think "recent" applies here?
>
> I didn't look closely, but it appears that the sctp procfs file is
> world-readable. So we gave unprivileged userspace the ability to leak
> kernel memory?
>
> So I'm thinking that we aim for 5.12-rc1 on all three patches with a cc:stable?
I'd rather take the sctp patch sooner, we'll send another batch
of networking fixes for 5.11, anyway. Would that be okay with you?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some seq_file users that were recently broken
2021-02-06 22:29 ` Jakub Kicinski
@ 2021-02-07 21:11 ` Andrew Morton
2021-02-08 19:42 ` Jakub Kicinski
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2021-02-07 21:11 UTC (permalink / raw)
To: Jakub Kicinski
Cc: NeilBrown, Alexander Viro, Jonathan Corbet, Xin Long,
linux-kernel, linux-doc, Ingo Molnar, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
On Sat, 6 Feb 2021 14:29:24 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 5 Feb 2021 14:35:50 -0800 Andrew Morton wrote:
> > On Fri, 05 Feb 2021 11:36:30 +1100 NeilBrown <neilb@suse.de> wrote:
> >
> > > A recent change to seq_file broke some users which were using seq_file
> > > in a non-"standard" way ... though the "standard" isn't documented, so
> > > they can be excused. The result is a possible leak - of memory in one
> > > case, of references to a 'transport' in the other.
> > >
> > > These three patches:
> > > 1/ document and explain the problem
> > > 2/ fix the problem user in x86
> > > 3/ fix the problem user in net/sctp
> >
> > 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
> > interface") was August 2018, so I don't think "recent" applies here?
> >
> > I didn't look closely, but it appears that the sctp procfs file is
> > world-readable. So we gave unprivileged userspace the ability to leak
> > kernel memory?
> >
> > So I'm thinking that we aim for 5.12-rc1 on all three patches with a cc:stable?
>
> I'd rather take the sctp patch sooner, we'll send another batch
> of networking fixes for 5.11, anyway. Would that be okay with you?
Sure.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some seq_file users that were recently broken
2021-02-07 21:11 ` Andrew Morton
@ 2021-02-08 19:42 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2021-02-08 19:42 UTC (permalink / raw)
To: Andrew Morton
Cc: NeilBrown, Alexander Viro, Jonathan Corbet, Xin Long,
linux-kernel, linux-doc, Ingo Molnar, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
On Sun, 7 Feb 2021 13:11:45 -0800 Andrew Morton wrote:
> On Sat, 6 Feb 2021 14:29:24 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> > On Fri, 5 Feb 2021 14:35:50 -0800 Andrew Morton wrote:
> > > On Fri, 05 Feb 2021 11:36:30 +1100 NeilBrown <neilb@suse.de> wrote:
> > > 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
> > > interface") was August 2018, so I don't think "recent" applies here?
> > >
> > > I didn't look closely, but it appears that the sctp procfs file is
> > > world-readable. So we gave unprivileged userspace the ability to leak
> > > kernel memory?
> > >
> > > So I'm thinking that we aim for 5.12-rc1 on all three patches with a cc:stable?
> >
> > I'd rather take the sctp patch sooner, we'll send another batch
> > of networking fixes for 5.11, anyway. Would that be okay with you?
>
> Sure.
Applied patch 3 to net, thanks everyone!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some seq_file users that were recently broken
2021-02-05 22:35 ` [PATCH 0/3] Fix some seq_file users that were recently broken Andrew Morton
2021-02-06 22:29 ` Jakub Kicinski
@ 2021-02-07 22:45 ` NeilBrown
1 sibling, 0 replies; 8+ messages in thread
From: NeilBrown @ 2021-02-07 22:45 UTC (permalink / raw)
To: Andrew Morton
Cc: Alexander Viro, Jonathan Corbet, Xin Long, linux-kernel,
linux-doc, Ingo Molnar, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Vlad Yasevich, Neil Horman,
Marcelo Ricardo Leitner, David S. Miller, linux-sctp, netdev
[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]
On Fri, Feb 05 2021, Andrew Morton wrote:
> On Fri, 05 Feb 2021 11:36:30 +1100 NeilBrown <neilb@suse.de> wrote:
>
>> A recent change to seq_file broke some users which were using seq_file
>> in a non-"standard" way ... though the "standard" isn't documented, so
>> they can be excused. The result is a possible leak - of memory in one
>> case, of references to a 'transport' in the other.
>>
>> These three patches:
>> 1/ document and explain the problem
>> 2/ fix the problem user in x86
>> 3/ fix the problem user in net/sctp
>>
>
> 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and
> interface") was August 2018, so I don't think "recent" applies here?
I must be getting old :-(
>
> I didn't look closely, but it appears that the sctp procfs file is
> world-readable. So we gave unprivileged userspace the ability to leak
> kernel memory?
Not quite that bad. The x86 problem allows arbitrary memory to be
leaked, but that is in debugfs (as I'm sure you saw) so is root-only.
The sctp one only allows an sctp_transport to be pinned. That is not
good and would, e.g., prevent the module from being unloaded. But
unlike a simple memory leak it won't affect anything outside of sctp.
>
> So I'm thinking that we aim for 5.12-rc1 on all three patches with a cc:stable?
Thanks for looking after these!
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-08 19:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 0:36 [PATCH 0/3] Fix some seq_file users that were recently broken NeilBrown
2021-02-05 0:36 ` [PATCH 3/3] net: fix iteration for sctp transport seq_files NeilBrown
2021-02-05 14:32 ` Marcelo Ricardo Leitner
2021-02-05 22:35 ` [PATCH 0/3] Fix some seq_file users that were recently broken Andrew Morton
2021-02-06 22:29 ` Jakub Kicinski
2021-02-07 21:11 ` Andrew Morton
2021-02-08 19:42 ` Jakub Kicinski
2021-02-07 22:45 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).