[PATCH 0/3] Fix some seq_file users that were recently broken

[PATCH 0/3] Fix some seq_file users that were recently broken

[NeilBrown]

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

[PATCH 3/3] net: fix iteration for sctp transport seq_files

[NeilBrown]

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;
 }
 

Re: [PATCH 3/3] net: fix iteration for sctp transport seq_files

[Marcelo Ricardo Leitner]

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>

Re: [PATCH 0/3] Fix some seq_file users that were recently broken

[Andrew Morton]

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?

Re: [PATCH 0/3] Fix some seq_file users that were recently broken

[Jakub Kicinski]

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?

Re: [PATCH 0/3] Fix some seq_file users that were recently broken

[Andrew Morton]

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.

Re: [PATCH 0/3] Fix some seq_file users that were recently broken

[NeilBrown]

[-- 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 --]

Re: [PATCH 0/3] Fix some seq_file users that were recently broken

[Jakub Kicinski]

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!