* 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).