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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YWQ43VyG8bF2gvF7@t14s.localdomain>
Date:   Mon, 11 Oct 2021 10:15:09 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-sctp@...r.kernel.org,
        netdev@...r.kernel.org, Eiichi Tsukata <eiichi.tsukata@...anix.com>
Subject: Re: [PATCH] sctp: account stream padding length for reconf chunk

On Mon, Oct 11, 2021 at 08:42:57AM +0200, Greg KH wrote:
> From: Eiichi Tsukata <eiichi.tsukata@...anix.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@...il.com>
> Cc: Neil Horman <nhorman@...driver.com>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: linux-sctp@...r.kernel.org
> Cc: netdev@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Fixes: cc16f00f6529 ("sctp: add support for generating stream reconf ssn reset request chunk")
> Reported-by: syzbot <syzkaller@...glegroups.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@...r.kernel.org>
> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@...anix.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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;