* Re: [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing [not found] <5955bc857c93d4bb64731ef7a9e90cb0094a8989.1595450200.git.marcelo.leitner@gmail.com> @ 2020-07-22 20:42 ` Marcelo Ricardo Leitner 2020-07-23 9:22 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Marcelo Ricardo Leitner @ 2020-07-22 20:42 UTC (permalink / raw) To: netdev; +Cc: Neil Horman, Christoph Hellwig, linux-sctp Cc'ing linux-sctp@vger.kernel.org. On Wed, Jul 22, 2020 at 05:38:58PM -0300, Marcelo Ricardo Leitner wrote: > This sockopt accepts two kinds of parameters, using struct > sctp_sack_info and struct sctp_assoc_value. The mentioned commit didn't > notice an implicit cast from the smaller (latter) struct to the bigger > one (former) when copying the data from the user space, which now leads > to an attempt to write beyond the buffer (because it assumes the storing > buffer is bigger than the parameter itself). > > Fix it by giving it a special buffer if the smaller struct is used by > the application. > > Fixes: ebb25defdc17 ("sctp: pass a kernel pointer to sctp_setsockopt_delayed_ack") > Reported-by: syzbot+0e4699d000d8b874d8dc@syzkaller.appspotmail.com > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > net/sctp/socket.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 9a767f35971865f46b39131fc8d96d8c3c2aa1a8..b71c36af7687247b4fc9e160219b76f5c41b2fe2 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -2756,6 +2756,7 @@ static int sctp_setsockopt_delayed_ack(struct sock *sk, > { > struct sctp_sock *sp = sctp_sk(sk); > struct sctp_association *asoc; > + struct sctp_sack_info _params; > > if (optlen = sizeof(struct sctp_sack_info)) { > if (params->sack_delay = 0 && params->sack_freq = 0) > @@ -2767,7 +2768,9 @@ static int sctp_setsockopt_delayed_ack(struct sock *sk, > "Use struct sctp_sack_info instead\n", > current->comm, task_pid_nr(current)); > > - if (params->sack_delay = 0) > + memcpy(&_params, params, sizeof(struct sctp_assoc_value)); > + params = &_params; > + if (((struct sctp_assoc_value *)params)->assoc_value = 0) > params->sack_freq = 1; > else > params->sack_freq = 0; > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing 2020-07-22 20:42 ` [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing Marcelo Ricardo Leitner @ 2020-07-23 9:22 ` Christoph Hellwig 2020-07-23 15:30 ` Marcelo Ricardo Leitner 0 siblings, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2020-07-23 9:22 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: netdev, Neil Horman, Christoph Hellwig, linux-sctp On Wed, Jul 22, 2020 at 05:42:31PM -0300, Marcelo Ricardo Leitner wrote: > Cc'ing linux-sctp@vger.kernel.org. What do you think of this version, which I think is a little cleaner? diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 9a767f35971865..6ce460428af9f3 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -2749,31 +2749,12 @@ static void sctp_apply_asoc_delayed_ack(struct sctp_sack_info *params, * timer to expire. The default value for this is 2, setting this * value to 1 will disable the delayed sack algorithm. */ - -static int sctp_setsockopt_delayed_ack(struct sock *sk, - struct sctp_sack_info *params, - unsigned int optlen) +static int __sctp_setsockopt_delayed_ack(struct sock *sk, + struct sctp_sack_info *params) { struct sctp_sock *sp = sctp_sk(sk); struct sctp_association *asoc; - if (optlen = sizeof(struct sctp_sack_info)) { - if (params->sack_delay = 0 && params->sack_freq = 0) - return 0; - } else if (optlen = sizeof(struct sctp_assoc_value)) { - pr_warn_ratelimited(DEPRECATED - "%s (pid %d) " - "Use of struct sctp_assoc_value in delayed_ack socket option.\n" - "Use struct sctp_sack_info instead\n", - current->comm, task_pid_nr(current)); - - if (params->sack_delay = 0) - params->sack_freq = 1; - else - params->sack_freq = 0; - } else - return -EINVAL; - /* Validate value parameter. */ if (params->sack_delay > 500) return -EINVAL; @@ -2821,6 +2802,31 @@ static int sctp_setsockopt_delayed_ack(struct sock *sk, return 0; } +static int sctp_setsockopt_delayed_ack(struct sock *sk, + struct sctp_sack_info *params, + unsigned int optlen) +{ + if (optlen = sizeof(struct sctp_assoc_value)) { + struct sctp_sack_info p; + + pr_warn_ratelimited(DEPRECATED + "%s (pid %d) " + "Use of struct sctp_assoc_value in delayed_ack socket option.\n" + "Use struct sctp_sack_info instead\n", + current->comm, task_pid_nr(current)); + + memcpy(&p, params, sizeof(struct sctp_assoc_value)); + p.sack_freq = p.sack_delay ? 0 : 1; + return __sctp_setsockopt_delayed_ack(sk, &p); + } + + if (optlen != sizeof(struct sctp_sack_info)) + return -EINVAL; + if (params->sack_delay = 0 && params->sack_freq = 0) + return 0; + return __sctp_setsockopt_delayed_ack(sk, params); +} + /* 7.1.3 Initialization Parameters (SCTP_INITMSG) * * Applications can specify protocol parameters for the default association ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing 2020-07-23 9:22 ` Christoph Hellwig @ 2020-07-23 15:30 ` Marcelo Ricardo Leitner 2020-07-24 6:46 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Marcelo Ricardo Leitner @ 2020-07-23 15:30 UTC (permalink / raw) To: Christoph Hellwig; +Cc: netdev, Neil Horman, linux-sctp On Thu, Jul 23, 2020 at 11:22:38AM +0200, Christoph Hellwig wrote: > On Wed, Jul 22, 2020 at 05:42:31PM -0300, Marcelo Ricardo Leitner wrote: > > Cc'ing linux-sctp@vger.kernel.org. > > What do you think of this version, which I think is a little cleaner? It splits up the argument parsing from the actual handling, ok. Looks good. Just one point: > +static int sctp_setsockopt_delayed_ack(struct sock *sk, > + struct sctp_sack_info *params, > + unsigned int optlen) > +{ > + if (optlen = sizeof(struct sctp_assoc_value)) { > + struct sctp_sack_info p; > + > + pr_warn_ratelimited(DEPRECATED > + "%s (pid %d) " > + "Use of struct sctp_assoc_value in delayed_ack socket option.\n" > + "Use struct sctp_sack_info instead\n", > + current->comm, task_pid_nr(current)); > + > + memcpy(&p, params, sizeof(struct sctp_assoc_value)); > + p.sack_freq = p.sack_delay ? 0 : 1; Please add a comment saying that sctp_sack_info.sack_delay maps exactly to sctp_assoc_value.assoc_value, so that's why we can do memcpy and read assoc_value as sack_delay. I think it will help us not trip on this again in the future. > + return __sctp_setsockopt_delayed_ack(sk, &p); > + } > + > + if (optlen != sizeof(struct sctp_sack_info)) > + return -EINVAL; > + if (params->sack_delay = 0 && params->sack_freq = 0) > + return 0; > + return __sctp_setsockopt_delayed_ack(sk, params); > +} > + > /* 7.1.3 Initialization Parameters (SCTP_INITMSG) > * > * Applications can specify protocol parameters for the default association ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing 2020-07-23 15:30 ` Marcelo Ricardo Leitner @ 2020-07-24 6:46 ` Christoph Hellwig 0 siblings, 0 replies; 4+ messages in thread From: Christoph Hellwig @ 2020-07-24 6:46 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Christoph Hellwig, netdev, Neil Horman, linux-sctp On Thu, Jul 23, 2020 at 12:30:25PM -0300, Marcelo Ricardo Leitner wrote: > On Thu, Jul 23, 2020 at 11:22:38AM +0200, Christoph Hellwig wrote: > > On Wed, Jul 22, 2020 at 05:42:31PM -0300, Marcelo Ricardo Leitner wrote: > > > Cc'ing linux-sctp@vger.kernel.org. > > > > What do you think of this version, which I think is a little cleaner? > > It splits up the argument parsing from the actual handling, ok. Looks > good. Just one point: > > > +static int sctp_setsockopt_delayed_ack(struct sock *sk, > > + struct sctp_sack_info *params, > > + unsigned int optlen) > > +{ > > + if (optlen = sizeof(struct sctp_assoc_value)) { > > + struct sctp_sack_info p; > > + > > + pr_warn_ratelimited(DEPRECATED > > + "%s (pid %d) " > > + "Use of struct sctp_assoc_value in delayed_ack socket option.\n" > > + "Use struct sctp_sack_info instead\n", > > + current->comm, task_pid_nr(current)); > > + > > + memcpy(&p, params, sizeof(struct sctp_assoc_value)); > > + p.sack_freq = p.sack_delay ? 0 : 1; > > Please add a comment saying that sctp_sack_info.sack_delay maps > exactly to sctp_assoc_value.assoc_value, so that's why we can do > memcpy and read assoc_value as sack_delay. I think it will help us not > trip on this again in the future. Yeah. Actually I think I'll go all the way and kill the not very obvious or type safe memcpy as well. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-24 6:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <5955bc857c93d4bb64731ef7a9e90cb0094a8989.1595450200.git.marcelo.leitner@gmail.com> 2020-07-22 20:42 ` [PATCH net-next] sctp: fix slab-out-of-bounds in SCTP_DELAYED_SACK processing Marcelo Ricardo Leitner 2020-07-23 9:22 ` Christoph Hellwig 2020-07-23 15:30 ` Marcelo Ricardo Leitner 2020-07-24 6:46 ` Christoph Hellwig
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).