On Thu, Jul 13, 2017 at 8:32 PM, David Miller <da...@davemloft.net> wrote:
> From: Alexander Potapenko <gli...@google.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
Do we need to bother about truncated structures? Shouldn't it be
enough to check that there's at least sizeof(struct sctp_paramhdr)
bytes left then?
> and
>
> (void *)err + offsetof(err, length) + sizeof(err->length) <= (void *)
> chunk_hdr + end
>
> And yeah, that isn't exactly concise nor pretty...
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
