LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Wenwen Wang <wang6495@umn.edu>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
	Kangjie Lu <kjlu@umn.edu>, Vlad Yasevich <vyasevich@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"open list:SCTP PROTOCOL" <linux-sctp@vger.kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sctp: fix a potential missing-check bug
Date: Thu, 3 May 2018 07:23:41 -0400	[thread overview]
Message-ID: <20180503112341.GA4220@hmswarspite.think-freely.org> (raw)
In-Reply-To: <CAAa=b7d_dHFUudPdJr3YuDaYRzNvAgHd0Lcubw3Ks94O-2kb3w@mail.gmail.com>

On Wed, May 02, 2018 at 08:07:17PM -0500, Wenwen Wang wrote:
> Hi Marcelo,
> 
> I guess I worked on an old version of the kernel. I will re-submit the
> patch. Sorry :(
> 
You don't have to resubmit the patch, this isn't broken.  As marcelo points out,
a value of zero in this socket option is special, meaning set the fragmentation
to whatever the pmtu is, which will always rest between the min and max segment
lengths.

Neil

> Wenwen
> 
> On Wed, May 2, 2018 at 6:23 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > Hi Wenwen,
> >
> > On Wed, May 02, 2018 at 05:12:45PM -0500, Wenwen Wang wrote:
> >> In sctp_setsockopt_maxseg(), the integer 'val' is compared against min_len
> >> and max_len to check whether it is in the appropriate range. If it is not,
> >> an error code -EINVAL will be returned. This is enforced by a security
> >> check. But, this check is only executed when 'val' is not 0. In fact, if
> >
> > Which makes sense, no? Especially if considering that 0 should be an
> > allowed value as it turns off the user limit.
> >
> >> 'val' is 0, it will be assigned with a new value (if the return value of
> >> the function sctp_id2assoc() is not 0) in the following execution. However,
> >> this new value of 'val' is not checked before it is used to assigned to
> >
> > Which 'new value'? val is not set to something new during the
> > function. It always contains the user supplied value.
> >
> >> asoc->user_frag. That means it is possible that the new value of 'val'
> >> could be out of the expected range. This can cause security issues
> >> such as buffer overflows, e.g., the new value of 'val' is used as an index
> >> to access a buffer.
> >>
> >> This patch inserts a check for the new value of 'val' to see if it is in
> >> the expected range. If it is not, an error code -EINVAL will be returned.
> >>
> >> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> >> ---
> >>  net/sctp/socket.c | 21 ++++++++++-----------
> >>  1 file changed, 10 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 80835ac..2beb601 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -3212,6 +3212,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
> >>       struct sctp_af *af = sp->pf->af;
> >>       struct sctp_assoc_value params;
> >>       struct sctp_association *asoc;
> >> +     int min_len, max_len;
> >>       int val;
> >>
> >>       if (optlen == sizeof(int)) {
> >> @@ -3231,19 +3232,15 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
> >>               return -EINVAL;
> >>       }
> >>
> >> -     if (val) {
> >> -             int min_len, max_len;
> >> +     min_len = SCTP_DEFAULT_MINSEGMENT - af->net_header_len;
> >> +     min_len -= af->ip_options_len(sk);
> >> +     min_len -= sizeof(struct sctphdr) +
> >> +                sizeof(struct sctp_data_chunk);
> >
> > On which tree did you base your patch on? Your patch lacks a tag so it
> > defaults to net-next, and I reworked this section on current net-next
> > and these MTU calculcations are now handled by sctp_mtu_payload().
> >
> > But even for net tree, I don't understand which issue you're fixing
> > here. Actually it seems to me that both codes seems to do the same
> > thing.
> >
> >>
> >> -             min_len = SCTP_DEFAULT_MINSEGMENT - af->net_header_len;
> >> -             min_len -= af->ip_options_len(sk);
> >> -             min_len -= sizeof(struct sctphdr) +
> >> -                        sizeof(struct sctp_data_chunk);
> >> +     max_len = SCTP_MAX_CHUNK_LEN - sizeof(struct sctp_data_chunk);
> >>
> >> -             max_len = SCTP_MAX_CHUNK_LEN - sizeof(struct sctp_data_chunk);
> >> -
> >> -             if (val < min_len || val > max_len)
> >> -                     return -EINVAL;
> >> -     }
> >> +     if (val && (val < min_len || val > max_len))
> >> +             return -EINVAL;
> >>
> >>       asoc = sctp_id2assoc(sk, params.assoc_id);
> >>       if (asoc) {
> >> @@ -3253,6 +3250,8 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
> >>                       val -= sizeof(struct sctphdr) +
> >>                              sctp_datachk_len(&asoc->stream);
> >>               }
> >> +             if (val < min_len || val > max_len)
> >> +                     return -EINVAL;
> >>               asoc->user_frag = val;
> >>               asoc->frag_point = sctp_frag_point(asoc, asoc->pathmtu);
> >>       } else {
> >> --
> >> 2.7.4
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 

  reply	other threads:[~2018-05-03 11:24 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02 22:12 Wenwen Wang
2018-05-02 23:23 ` Marcelo Ricardo Leitner
2018-05-03  1:07   ` Wenwen Wang
2018-05-03 11:23     ` Neil Horman [this message]
2018-05-03  1:15 Wenwen Wang
2018-05-03  1:24 ` Marcelo Ricardo Leitner
2018-05-03  1:27   ` Wenwen Wang
2018-05-03  1:48     ` Marcelo Ricardo Leitner
2018-05-03 12:01       ` Wenwen Wang
2018-05-03 12:46         ` Marcelo Ricardo Leitner
2018-05-03 13:31           ` Wenwen Wang
2018-05-03 13:39             ` Marcelo Ricardo Leitner
2018-05-03 13:43               ` Wenwen Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180503112341.GA4220@hmswarspite.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=davem@davemloft.net \
    --cc=kjlu@umn.edu \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=vyasevich@gmail.com \
    --cc=wang6495@umn.edu \
    --subject='Re: [PATCH] sctp: fix a potential missing-check bug' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).