All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
To: Richard Weinberger <richard@nod.at>
Cc: linux-can <linux-can@vger.kernel.org>
Subject: PATCH: Breaking UAPI change?
Date: Wed, 24 Mar 2021 20:27:46 +0100	[thread overview]
Message-ID: <20210324192746.GA7408@x1.vandijck-laurijssen.be> (raw)
In-Reply-To: <1135648123.112255.1616613706554.JavaMail.zimbra@nod.at>

> ----- Ursprüngliche Mail -----
> >> commit f5223e9eee65 ("can: extend sockaddr_can to include j1939 members")
> >> increased the size of
> >> struct sockaddr_can.
> >> This is a problem for applications which use recvfrom() with addrlen being
> >> sizeof(struct sockaddr_can)
> >> or sizeof(struct sockaddr).
> >> If such an application was built before the change it will no longer function
> >> correctly on newer kernels.
> > 
> > This scenario was identified, and explicitely dealt with.
> > This requires a tiny bit different code, i.e. net/can/raw.c should use
> > REQUIRED_SIZE() instead of sizeof() for testing the size of the address
> > structure.
> > 
> >> In fact I ran into such a scenario and found the said commit later that day.
> > 
> > Looking to the v5.10 kernel (which I happen to have checked out),
> > your claim indeed seems true, the raw_recvmsg does not (raw_bind and
> > raw_sendmsg work correct, but that's not important for your problem).
> > 
> >> 
> >> Is this a known issue?
> > 
> > It wasn't, until you found it :-)
> 
> Thanks for the prompt reply!
>  
> >> Or is this allowed and application must not use sizeof(struct sockaddr_can) as
> >> addrlen?
> > 
> > sizeof(struct sockaddr_can). As you already mentioned, applications may have
> > been built
> > before the size increase, and so they should not be recompiled.
> > 
> > 
> >> If so, what is the proposed way to avoid future breakage?
> > 
> > Your application should not change. Kernel must be fixed.
> 
> Feel free to CC me when you submit a patch, I'll happily test it.

Here it goes.
Even not compile tested :-( on my v5.10, at this hour.
I spotted a similar problem in getsockname/getpeername calls.

The patch will test for minimum required size before touching anything,
later on, the maximum size will be evaluated.

Happy testing?

commit 124900109ca88d29382ef2e2b848d3a2f9d67b98
Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
Date:   Wed Mar 24 20:08:50 2021

    can raw: don't increase provided name length
    
    The length of the buffer is known by the application,
    not the kernel. Kernel is supposed to return only the
    size of used bytes.
    There is a minimum required size for the struct sockaddr_can
    to be usefull for can_raw, so errors are returned when
    the provided size is lower
    
    Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

diff --git a/net/can/raw.c b/net/can/raw.c
index 6ec8aa1d0da4..00d352ae221e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -475,7 +475,7 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 	if (peer)
 		return -EOPNOTSUPP;
 
-	memset(addr, 0, sizeof(*addr));
+	memset(addr, 0, CAN_REQUIRED_SIZE(*addr, ifindex));
 	addr->can_family  = AF_CAN;
 	addr->can_ifindex = ro->ifindex;
 
@@ -806,6 +806,10 @@ static int raw_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 		return sock_recv_errqueue(sk, msg, size,
 					  SOL_CAN_RAW, SCM_CAN_RAW_ERRQUEUE);
 
+	if (msg->name && msg->msg_namelen <
+			CAN_REQUIRED_SIZE(struct sockaddr_can, ifindex))
+		return -EINVAL;
+
 	skb = skb_recv_datagram(sk, flags, noblock, &err);
 	if (!skb)
 		return err;
@@ -825,7 +829,8 @@ static int raw_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 
 	if (msg->msg_name) {
 		__sockaddr_check_size(sizeof(struct sockaddr_can));
-		msg->msg_namelen = sizeof(struct sockaddr_can);
+		if (msg->msg_namelen > sizeof(struct sockaddr_can))
+			msg->msg_namelen = sizeof(struct sockaddr_can);
 		memcpy(msg->msg_name, skb->cb, msg->msg_namelen);
 	}
 

  reply	other threads:[~2021-03-24 19:28 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 15:30 Breaking UAPI change? Richard Weinberger
2021-03-24 19:01 ` Kurt Van Dijck
2021-03-24 19:21   ` Richard Weinberger
2021-03-24 19:27     ` Kurt Van Dijck [this message]
2021-03-24 19:47       ` PATCH: " Richard Weinberger
2021-03-24 20:26       ` Oliver Hartkopp
2021-03-24 21:59         ` Oliver Hartkopp
2021-03-25  8:01         ` Kurt Van Dijck

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=20210324192746.GA7408@x1.vandijck-laurijssen.be \
    --to=dev.kurt@vandijck-laurijssen.be \
    --cc=linux-can@vger.kernel.org \
    --cc=richard@nod.at \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.