PATCH: Breaking UAPI change?

[Kurt Van Dijck <dev.kurt@xxxxxxxxxxxxxxxxxxxxxx>]

> ----- 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@xxxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxxx>

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);
 	}