linux-kernel - Re: [PATCH] sctp: don't dereference ptr before leaving _sctp_walk

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: <20170713.113206.177363709000549854.davem@davemloft.net>
Date:   Thu, 13 Jul 2017 11:32:06 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     glider@...gle.com
Cc:     dvyukov@...gle.com, kcc@...gle.com, edumazet@...gle.com,
        lucien.xin@...il.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] sctp: don't dereference ptr before leaving
 _sctp_walk_{params,errors}()

From: Alexander Potapenko <glider@...gle.com>
Date: Thu, 13 Jul 2017 20:10:34 +0200

> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index a9519a06a23b..f13632ee33f0 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -469,6 +469,7 @@ _sctp_walk_params((pos), (chunk), ntohs((chunk)->chunk_hdr.length), member)
>  
>  #define _sctp_walk_params(pos, chunk, end, member)\
>  for (pos.v = chunk->member;\
> +     pos.v < (void *)chunk + end &&\
>       pos.v <= (void *)chunk + end - ntohs(pos.p->length) &&\
>       ntohs(pos.p->length) >= sizeof(struct sctp_paramhdr);\
>       pos.v += SCTP_PAD4(ntohs(pos.p->length)))
> @@ -479,6 +480,7 @@ _sctp_walk_errors((err), (chunk_hdr), ntohs((chunk_hdr)->length))
>  #define _sctp_walk_errors(err, chunk_hdr, end)\
>  for (err = (sctp_errhdr_t *)((void *)chunk_hdr + \
>  	    sizeof(struct sctp_chunkhdr));\
> +     (void *)err <= (void *)chunk_hdr + end &&\
>       (void *)err <= (void *)chunk_hdr + end - ntohs(err->length) &&\
>       ntohs(err->length) >= sizeof(sctp_errhdr_t); \
>       err = (sctp_errhdr_t *)((void *)err + SCTP_PAD4(ntohs(err->length))))

Even with the "err < ..." fixed in the second hunk, I still think you need
to tweak these checks some more.

What is necessary is that the first two members of sctp_paramhdr or
sctp_errhdr are in the range ptr to end.

struct sctp_paramhdr {
	__be16 type;
	__be16 length;
};

typedef struct sctp_errhdr {
	__be16 cause;
	__be16 length;
	__u8  variable[0];
} sctp_errhdr_t;

so that we can legally dereference ->length.

But that is not what your new check is doing.  Your new check is only
making sure there is at least one byte there.

I guess it's not easy to write a clean test that doesn't hardcode the
offset of the length field and it's size.

Something like:

	pos.v + offsetof(pos.v, length) + sizeof(pos.v->length) <= (void *) chunk + end

and

	(void *)err + offsetof(err, length) + sizeof(err->length) <= (void *) chunk_hdr + end

And yeah, that isn't exactly concise nor pretty...