| Message ID | YNrXoNAiQama8Us8@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: prevent info leak in sctp_make_heartbeat() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | warning | WARNING: Unknown commit id 'fe59379b9ab7', maybe rebased or not pulled? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
Does that gcc extension work with all compilers, especially clang? Dan Carpenter wrote on 29.06.21 10:19: > 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 <dan.carpenter@oracle.com> > --- > 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,
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
> > - struct sctp_sender_hb_info hbinfo; > > + struct sctp_sender_hb_info hbinfo = {}; > Does that gcc extension work with all compilers, especially clang? > > 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)
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,
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 <dan.carpenter@oracle.com> --- 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(-)