[PATCH] sctp: account stream padding length for reconf chunk

[PATCH] sctp: account stream padding length for reconf chunk

[Greg KH]

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.

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>
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(
 	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)
-- 
2.33.0

Re: [PATCH] sctp: account stream padding length for reconf chunk

[Marcelo Ricardo Leitner]

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;
 

Re: [PATCH] sctp: account stream padding length for reconf chunk

[Eiichi Tsukata]

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

Re: [PATCH] sctp: account stream padding length for reconf chunk

[Marcelo Ricardo Leitner]

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