Re: [PATCH] sctp: don't dereference ptr before leaving _sctp_walk_{params,errors}()

[Alexander Potapenko]

On Thu, Jul 13, 2017 at 8:32 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: Alexander Potapenko <glider@xxxxxxxxxx>
> 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