Breaking UAPI change?

Breaking UAPI change?

[Richard Weinberger]

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?

Thanks,
//richard

Re: Breaking UAPI change?

[Kurt Van Dijck]

Hello,

On Wed, 24 Mar 2021 16:30:58 +0100, Richard Weinberger wrote:
> 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.

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

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

Kurt

Re: Breaking UAPI change?

[Richard Weinberger]

Kurt,

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

Thanks,
//richard

PATCH: Breaking UAPI change?

[Kurt Van Dijck]

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

Re: PATCH: Breaking UAPI change?

[Richard Weinberger]

Kurt,

----- Ursprüngliche Mail -----
> 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));

can_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 <

msg->msg_name?

> +			CAN_REQUIRED_SIZE(struct sockaddr_can, ifindex))
> +		return -EINVAL;
> +

With the above changes this -EINVAL always triggers.
msg->msg_namelen is 0 in my case.

The application does:
socklen_t addrlen = sizeof(struct sockaddr_can);
recvfrom(s, &frame, sizeof(struct can_frame), 0, (struct sockaddr *)&addr, &addrlen);

Thanks,
//richard

Re: PATCH: Breaking UAPI change?

[Oliver Hartkopp]

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

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

Re: PATCH: Breaking UAPI change?

[Oliver Hartkopp]

Hi Kurt,

I sent a patch which addresses the msg_name issue in all missing CAN 
protocols.

https://lore.kernel.org/linux-can/20210324215442.44537-1-socketcan@hartkopp.net/T/#u

Can you please check, if we should go for this approach?

@Richard: Can you please test this patch if it fixes your issue?

Regards,
Oliver

On 24.03.21 21:26, Oliver Hartkopp wrote:
> 
> 
> 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@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;
> 
> 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);
>>       }
>>

Re: PATCH: Breaking UAPI change?

[Kurt Van Dijck]

On Wed, 24 Mar 2021 21:26:19 +0100, Oliver Hartkopp wrote:
> On 24.03.21 20:27, Kurt Van Dijck wrote:
> >>----- Ursprüngliche Mail -----
> >>>>commit f5223e9eee65 ("can: extend sockaddr_can to include j1939 members")
> >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;
> 
> Is there no need to adapt the return value then?
> 
> - return sizeof(*addr);
> + return CAN_REQUIRED_SIZE(*addr, ifindex);

indeed. I have missed that one.
I was looking for a function parameter, and then forgot somehow ...

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