Breaking UAPI change?

Hi!

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.

In fact I ran into such a scenario and found the said commit later that day.

Is this a known issue?

Or is this allowed and application must not use sizeof(struct sockaddr_can) as addrlen?

If so, what is the proposed way to avoid future breakage?

Quoted 8 lines

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.

Quoted 1 line

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

Quoted 2 lines

Is this a known issue?

It wasn't, until you found it :-)

Quoted 2 lines

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.

Quoted 1 line

If so, what is the proposed way to avoid future breakage?

Your application should not change. Kernel must be fixed.

----- Ursprüngliche Mail -----

Quoted 16 lines

Quoted 8 lines

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.

Quoted 1 line

In fact I ran into such a scenario and found the said commit later that day.

Quoted 8 lines

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

Quoted 2 lines

Is this a known issue?

It wasn't, until you found it :-)

Thanks for the prompt reply!

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Happy testing?

commit 124900109ca88d29382ef2e2b848d3a2f9d67b98

Author: Kurt Van Dijck <[email protected]>
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 <[email protected]>

net/can/raw.c +0 -0

index 6ec8aa1d0da4..00d352ae221e 100644

Quoted 36 lines

----- Ursprüngliche Mail -----

Quoted 16 lines

Quoted 8 lines

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.

Quoted 1 line

In fact I ran into such a scenario and found the said commit later that day.

Quoted 8 lines

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

Quoted 2 lines

Is this a known issue?

It wasn't, until you found it :-)

Thanks for the prompt reply!

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

net/can/raw.c +0 -0

index 6ec8aa1d0da4..00d352ae221e 100644

net/can/raw.c +6 -1

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

Quoted 68 lines · Kurt Van Dijck

Quoted 35 lines

----- Ursprüngliche Mail -----

Quoted 16 lines

Quoted 8 lines

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.

Quoted 1 line

In fact I ran into such a scenario and found the said commit later that day.

Quoted 8 lines

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

Quoted 2 lines

Is this a known issue?

It wasn't, until you found it :-)

Thanks for the prompt reply!

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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 <[email protected]> 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 <[email protected]>

net/can/raw.c +0 -0

index 6ec8aa1d0da4..00d352ae221e 100644

net/can/raw.c static int raw_getname(struct socket *sock, struct · line 480

Quoted 7 lines

net/can/raw.c +1 -1

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

Is there no need to adapt the return value then?

- return sizeof(*addr);
+ return CAN_REQUIRED_SIZE(*addr, ifindex);

Regards,
Oliver

ps. If so, I need to go through isotp_getname() too ...

Quoted 21 lines

net/can/raw.c +6 -1

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

Quoted 20 lines · Kurt Van Dijck

Quoted 2 lines

----- Ursprüngliche Mail -----

Quoted 1 line

Quoted 1 line

commit f5223e9eee65 ("can: extend sockaddr_can to include j1939 members")

commit 124900109ca88d29382ef2e2b848d3a2f9d67b98

Author: Kurt Van Dijck <[email protected]>
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 <[email protected]>

net/can/raw.c +0 -0

index 6ec8aa1d0da4..00d352ae221e 100644

net/can/raw.c static int raw_getname(struct socket *sock, struct sockaddr *uaddr, · line 479

Quoted 6 lines

net/can/raw.c +1 -1

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

Is there no need to adapt the return value then?

- return sizeof(*addr);
+ return CAN_REQUIRED_SIZE(*addr, ifindex);

Regards,
Oliver

ps. If so, I need to go through isotp_getname() too ...

Quoted 19 lines

net/can/raw.c +6 -1

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