On 24.03.21 20:27, Kurt Van Dijck wrote:
----- 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;
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 ...
@@ -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);
}
