linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Fix some seq_file users that were recently broken
@ 2021-02-05  0:36 NeilBrown
  2021-02-05  0:36 ` [PATCH 2/3] x86: fix seq_file iteration for pat/memtype.c NeilBrown
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ 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] 11+ messages in thread

* [PATCH 1/3] seq_file: document how per-entry resources are managed.
  2021-02-05  0:36 [PATCH 0/3] Fix some seq_file users that were recently broken NeilBrown
  2021-02-05  0:36 ` [PATCH 2/3] x86: fix seq_file iteration for pat/memtype.c NeilBrown
@ 2021-02-05  0:36 ` NeilBrown
  2021-02-05  2:20   ` Matthew Wilcox
  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
  3 siblings, 1 reply; 11+ 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

Users of seq_file will sometimes find it convenient to take a resource,
such as a lock or memory allocation, in the ->start or ->next
operations.
These are per-entry resources, distinct from per-session resources which
are taken in ->start and released in ->stop.

The preferred management of these is release the resource on the
subsequent call to ->next or ->stop.

However prior to Commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file
iteration code and interface") it happened that ->show would always be
called after ->start or ->next, and a few users chose to release the
resource in ->show.

This is no longer reliable.  Since the mentioned commit, ->next will
always come after a successful ->show (to ensure m->index is updated
correctly), so the original ordering cannot be maintained.

This patch updates the documentation to clearly state the required
behaviour.  Other patches will fix the few problematic users.

Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
Cc: Xin Long <lucien.xin@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 Documentation/filesystems/seq_file.rst |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/filesystems/seq_file.rst b/Documentation/filesystems/seq_file.rst
index 56856481dc8d..0e40e1532e7f 100644
--- a/Documentation/filesystems/seq_file.rst
+++ b/Documentation/filesystems/seq_file.rst
@@ -217,6 +217,12 @@ between the calls to start() and stop(), so holding a lock during that time
 is a reasonable thing to do. The seq_file code will also avoid taking any
 other locks while the iterator is active.
 
+The iterater value returned by start() or next() is guaranteed to be
+passed to a subsequenct next() or stop() call.  This allows resources
+such as locks that were taken to be reliably released.  There is *no*
+guarantee that the iterator will be passed to show(), though in practice
+it often will be.
+
 
 Formatted output
 ================



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] x86: fix seq_file iteration for pat/memtype.c
  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  0:36 ` [PATCH 1/3] seq_file: document how per-entry resources are managed NeilBrown
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: NeilBrown @ 2021-02-05  0:36 UTC (permalink / raw)
  To: Andrew Morton, Alexander Viro, Ingo Molnar, Dave Hansen,
	Andy Lutomirski, Peter Zijlstra
  Cc: Xin Long, linux-kernel

The memtype seq_file iterator allocates a buffer in the ->start and
->next functions and frees it in the ->show function.
The preferred handling for such resources is to free 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 memory.

So move the freeing of the buffer to ->next and ->stop.

Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
Cc: Xin Long <lucien.xin@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 arch/x86/mm/pat/memtype.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 8f665c352bf0..ca311aaa67b8 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -1164,12 +1164,14 @@ static void *memtype_seq_start(struct seq_file *seq, loff_t *pos)
 
 static void *memtype_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
+	kfree(v);
 	++*pos;
 	return memtype_get_idx(*pos);
 }
 
 static void memtype_seq_stop(struct seq_file *seq, void *v)
 {
+	kfree(v);
 }
 
 static int memtype_seq_show(struct seq_file *seq, void *v)
@@ -1181,8 +1183,6 @@ static int memtype_seq_show(struct seq_file *seq, void *v)
 			entry_print->end,
 			cattr_name(entry_print->type));
 
-	kfree(entry_print);
-
 	return 0;
 }
 



^ permalink raw reply related	[flat|nested] 11+ 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 ` [PATCH 2/3] x86: fix seq_file iteration for pat/memtype.c NeilBrown
  2021-02-05  0:36 ` [PATCH 1/3] seq_file: document how per-entry resources are managed 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
  3 siblings, 1 reply; 11+ 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] 11+ messages in thread

* Re: [PATCH 1/3] seq_file: document how per-entry resources are managed.
  2021-02-05  0:36 ` [PATCH 1/3] seq_file: document how per-entry resources are managed NeilBrown
@ 2021-02-05  2:20   ` Matthew Wilcox
  0 siblings, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2021-02-05  2:20 UTC (permalink / raw)
  To: NeilBrown
  Cc: Andrew Morton, Alexander Viro, Jonathan Corbet, Xin Long,
	linux-kernel, linux-doc

On Fri, Feb 05, 2021 at 11:36:30AM +1100, NeilBrown wrote:
> +passed to a subsequenct next() or stop() call.  This allows resources

s/quenct/quent/


^ permalink raw reply	[flat|nested] 11+ 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; 11+ 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] 11+ 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
                   ` (2 preceding siblings ...)
  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
  3 siblings, 2 replies; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread

end of thread, other threads:[~2021-02-08 20:54 UTC | newest]

Thread overview: 11+ 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 2/3] x86: fix seq_file iteration for pat/memtype.c NeilBrown
2021-02-05  0:36 ` [PATCH 1/3] seq_file: document how per-entry resources are managed NeilBrown
2021-02-05  2:20   ` Matthew Wilcox
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).