[PATCH net] sctp: prevent info leak in sctp_make_heartbeat()

Subsystems: networking [general], sctp protocol, the rest

STALE1893d

4 messages, 3 authors, 2021-06-29 · open the first message on its own page

[PATCH net] sctp: prevent info leak in sctp_make_heartbeat()

From: Dan Carpenter <hidden>
Date: 2021-06-29 08:20:32

The "hbinfo" struct has a 4 byte hole at the end so we have to zero it
out to prevent stack information from being disclosed.

Fixes: fe59379b9ab7 ("sctp: do the basic send and recv for PLPMTUD probe")
Signed-off-by: Dan Carpenter <redacted>
---
Btw = {} is the newest way to initialize holes.

In the past we have debated whether = {} will *always* zero out struct
holes and it wasn't clear from the C standard.  But it turns out that
"= {}" is not part of the standard but is instead a GCC extension and it
does clear the holes.  In GCC (not the C standard) then = {0}; is also
supposed to initialize holes in there was a bug in one version where it
didn't.

So that's nice, because adding memset()s to zero everywhere was ugly.

 net/sctp/sm_make_chunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 587fb3cb88e2..3a290f620e96 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1162,7 +1162,7 @@ struct sctp_chunk *sctp_make_new_encap_port(const struct sctp_association *asoc,
 struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
 				       const struct sctp_transport *transport)
 {
-	struct sctp_sender_hb_info hbinfo;
+	struct sctp_sender_hb_info hbinfo = {};
 	struct sctp_chunk *retval;
 
 	retval = sctp_make_control(asoc, SCTP_CID_HEARTBEAT, 0,
-- 
2.30.2

Re: [PATCH net] sctp: prevent info leak in sctp_make_heartbeat()

From: Andreas Fink <hidden>
Date: 2021-06-29 08:29:53

Does that gcc extension work with all compilers, especially clang?

Dan Carpenter wrote on 29.06.21 10:19:
quoted hunk
The "hbinfo" struct has a 4 byte hole at the end so we have to zero it
out to prevent stack information from being disclosed.

Fixes: fe59379b9ab7 ("sctp: do the basic send and recv for PLPMTUD probe")
Signed-off-by: Dan Carpenter <redacted>
---
Btw = {} is the newest way to initialize holes.

In the past we have debated whether = {} will *always* zero out struct
holes and it wasn't clear from the C standard.  But it turns out that
"= {}" is not part of the standard but is instead a GCC extension and it
does clear the holes.  In GCC (not the C standard) then = {0}; is also
supposed to initialize holes in there was a bug in one version where it
didn't.

So that's nice, because adding memset()s to zero everywhere was ugly.

 net/sctp/sm_make_chunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 587fb3cb88e2..3a290f620e96 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1162,7 +1162,7 @@ struct sctp_chunk *sctp_make_new_encap_port(const struct sctp_association *asoc,
 struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
 				       const struct sctp_transport *transport)
 {
-	struct sctp_sender_hb_info hbinfo;
+	struct sctp_sender_hb_info hbinfo = {};
 	struct sctp_chunk *retval;
 
 	retval = sctp_make_control(asoc, SCTP_CID_HEARTBEAT, 0,

Re: [PATCH net] sctp: prevent info leak in sctp_make_heartbeat()

From: Dan Carpenter <hidden>
Date: 2021-06-29 09:14:10

On Tue, Jun 29, 2021 at 10:23:49AM +0200, Andreas Fink wrote:
Does that gcc extension work with all compilers, especially clang?
I had to look up the thread where this was discussed.

https://lore.kernel.org/lkml/20200731045301.GI75549@unreal/

I was wrong.  It started as a GCC extension but was made part of the
standard in C11.  So we should be good.

https://lore.kernel.org/lkml/20200731140452.GE24045@ziepe.ca/

regards,
dan carpenter

RE: [PATCH net] sctp: prevent info leak in sctp_make_heartbeat()

From: David Laight <hidden>
Date: 2021-06-29 10:37:56

quoted
-	struct sctp_sender_hb_info hbinfo;
+	struct sctp_sender_hb_info hbinfo = {};
Does that gcc extension work with all compilers, especially clang?
quoted
So that's nice, because adding memset()s to zero everywhere was ugly.
The 'real fun' (tm) starts when the bit pattern for the NULL
pointer isn't 'all zeros'.
Using memset() is then broken - I suspect the compiler is
expected to initialise pointers to the correct NULL pattern.

Not that I think anyone sane would consider trying to compile
any 'normal' C code for such a system.

OTOH it is probably why clang is bleating about (int)((char *)0 + 4)
being undefined behaviour.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help