| Message ID | YWPc8Stn3iBBNh80@kroah.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sctp: account stream padding length for reconf chunk | expand |
On Mon, Oct 11, 2021 at 08:42:57AM +0200, Greg KH wrote: > From: Eiichi Tsukata <eiichi.tsukata@nutanix.com> > > "stream_len" is not always multiple of 4. Account padding length > which can be added in sctp_addto_chunk() for reconf chunk. This could be improved somehow. The real problem here is that the padding gets added twice by repeated calls to sctp_addto_chunk() while it's accounted only once in sctp_make_reconf() and yes, only shows up when "stream_len" is not a multiple of 4. > > Cc: Vlad Yasevich <vyasevich@gmail.com> > Cc: Neil Horman <nhorman@tuxdriver.com> > Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: linux-sctp@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Fixes: cc16f00f6529 ("sctp: add support for generating stream reconf ssn reset request chunk") > Reported-by: syzbot <syzkaller@googlegroups.com> Perhaps we can drop this Reported-by tag, as it was Eiichi that adapted a syzcaller repro to work on SCTP code. > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > net/sctp/sm_make_chunk.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index b8fa8f1a7277..f7a1072a2a2a 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -3694,8 +3694,8 @@ struct sctp_chunk *sctp_make_strreset_req( more context: __u16 stream_len = stream_num * sizeof(__u16); struct sctp_strreset_outreq outreq; struct sctp_strreset_inreq inreq; > struct sctp_chunk *retval; > __u16 outlen, inlen; > > - outlen = (sizeof(outreq) + stream_len) * out; > - inlen = (sizeof(inreq) + stream_len) * in; > + outlen = (sizeof(outreq) + SCTP_PAD4(stream_len)) * out; > + inlen = (sizeof(inreq) + SCTP_PAD4(stream_len)) * in; > > retval = sctp_make_reconf(asoc, outlen + inlen); > if (!retval) more context: return NULL; if (outlen) { outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST; outreq.param_hdr.length = htons(outlen); ^^^^^^^^^^^^^^^^^^^^^^ The issue with this is that on the receiving side, it will: sctp_process_strreset_outreq() ... nums = (ntohs(param.p->length) - sizeof(*outreq)) / sizeof(__u16); str_p = outreq->list_of_streams; for (i = 0; i < nums; i++) { if (ntohs(str_p[i]) >= stream->incnt) { So if stream_num was originally 1, stream_len would be 2, and with padding, 4. Here, nums would be 2 then, and not 1. The padding gets accounted as if it was payload. IOW, the patch is making the padding part of the parameter data by adding it to the header as well. SCTP padding works by having it in between them, and not inside them. This other approach avoids this issue by adding the padding only when allocating the packet. It (ab)uses the fact that inreq and outreq are already aligned to 4 bytes. Eiichi, can you please give it a go? struct sctp_strreset_outreq { struct sctp_paramhdr param_hdr; /* 0 4 */ __be32 request_seq; /* 4 4 */ __be32 response_seq; /* 8 4 */ __be32 send_reset_at_tsn; /* 12 4 */ __be16 list_of_streams[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 5 */ /* last cacheline: 16 bytes */ }; struct sctp_strreset_inreq { struct sctp_paramhdr param_hdr; /* 0 4 */ __be32 request_seq; /* 4 4 */ __be16 list_of_streams[]; /* 8 0 */ /* size: 8, cachelines: 1, members: 3 */ /* last cacheline: 8 bytes */ }; Thanks, Marcelo ---8<--- diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index b8fa8f1a7277..c7503fd64915 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -3697,7 +3697,7 @@ struct sctp_chunk *sctp_make_strreset_req( outlen = (sizeof(outreq) + stream_len) * out; inlen = (sizeof(inreq) + stream_len) * in; - retval = sctp_make_reconf(asoc, outlen + inlen); + retval = sctp_make_reconf(asoc, SCTP_PAD4(outlen) + SCTP_PAD4(inlen)); if (!retval) return NULL;
Hi Marcelo > On Oct 11, 2021, at 22:15, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > ... > > So if stream_num was originally 1, stream_len would be 2, and with > padding, 4. Here, nums would be 2 then, and not 1. The padding gets > accounted as if it was payload. > > IOW, the patch is making the padding part of the parameter data by > adding it to the header as well. SCTP padding works by having it in > between them, and not inside them. > > This other approach avoids this issue by adding the padding only when > allocating the packet. It (ab)uses the fact that inreq and outreq are > already aligned to 4 bytes. Eiichi, can you please give it a go? > > Thanks, I understood. I’ve tested your diff with my reproducer and it certainly works. Your diff looks good to me. > > ---8<--- > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index b8fa8f1a7277..c7503fd64915 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -3697,7 +3697,7 @@ struct sctp_chunk *sctp_make_strreset_req( > outlen = (sizeof(outreq) + stream_len) * out; > inlen = (sizeof(inreq) + stream_len) * in; > > - retval = sctp_make_reconf(asoc, outlen + inlen); > + retval = sctp_make_reconf(asoc, SCTP_PAD4(outlen) + SCTP_PAD4(inlen)); > if (!retval) > return NULL; Regards, Eiichi
On Tue, Oct 12, 2021 at 12:17:08AM +0000, Eiichi Tsukata wrote: > Hi Marcelo > > > On Oct 11, 2021, at 22:15, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > > > > ... > > > > So if stream_num was originally 1, stream_len would be 2, and with > > padding, 4. Here, nums would be 2 then, and not 1. The padding gets > > accounted as if it was payload. > > > > IOW, the patch is making the padding part of the parameter data by > > adding it to the header as well. SCTP padding works by having it in > > between them, and not inside them. > > > > This other approach avoids this issue by adding the padding only when > > allocating the packet. It (ab)uses the fact that inreq and outreq are > > already aligned to 4 bytes. Eiichi, can you please give it a go? > > > > > > Thanks, I understood. I’ve tested your diff with my reproducer and it certainly works. > Your diff looks good to me. Cool, thanks. I'm running a couple more tests on it and will submit it on your behalf by EOD if all goes well. Regards, Marcelo
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index b8fa8f1a7277..f7a1072a2a2a 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -3694,8 +3694,8 @@ struct sctp_chunk *sctp_make_strreset_req( struct sctp_chunk *retval; __u16 outlen, inlen; - outlen = (sizeof(outreq) + stream_len) * out; - inlen = (sizeof(inreq) + stream_len) * in; + outlen = (sizeof(outreq) + SCTP_PAD4(stream_len)) * out; + inlen = (sizeof(inreq) + SCTP_PAD4(stream_len)) * in; retval = sctp_make_reconf(asoc, outlen + inlen); if (!retval)