[PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Shigeru Yoshida]

tipc_udp_addr2str() should return non-zero value if the UDP media
address is invalid. Otherwise, a buffer overflow access can occur in
tipc_media_addr_printf(). Fix this by returning 1 on an invalid UDP
media address.

Fixes: d0f91938bede ("tipc: add ip/udp media type")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/tipc/udp_media.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index b849a3d133a0..439f75539977 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -135,8 +135,11 @@ static int tipc_udp_addr2str(struct tipc_media_addr *a, char *buf, int size)
 		snprintf(buf, size, "%pI4:%u", &ua->ipv4, ntohs(ua->port));
 	else if (ntohs(ua->proto) == ETH_P_IPV6)
 		snprintf(buf, size, "%pI6:%u", &ua->ipv6, ntohs(ua->port));
-	else
+	else {
 		pr_err("Invalid UDP media address\n");
+		return 1;
+	}
+
 	return 0;
 }
 
-- 
2.45.2

RE: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Tung Nguyen]

>tipc_udp_addr2str() should return non-zero value if the UDP media address is invalid. Otherwise, a buffer overflow access can occur in
>tipc_media_addr_printf(). Fix this by returning 1 on an invalid UDP media address.
>
>Fixes: d0f91938bede ("tipc: add ip/udp media type")
>Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>---
> net/tipc/udp_media.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index b849a3d133a0..439f75539977 100644
>--- a/net/tipc/udp_media.c
>+++ b/net/tipc/udp_media.c
>@@ -135,8 +135,11 @@ static int tipc_udp_addr2str(struct tipc_media_addr *a, char *buf, int size)
>                snprintf(buf, size, "%pI4:%u", &ua->ipv4, ntohs(ua->port));
>        else if (ntohs(ua->proto) == ETH_P_IPV6)
>                snprintf(buf, size, "%pI6:%u", &ua->ipv6, ntohs(ua->port));
>-       else
>+       else {
>                pr_err("Invalid UDP media address\n");
>+               return 1;
Please use -EINVAL instead.
>+       }
>+
>        return 0;
> }
>
>--
>2.45.2
>


The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions.

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Shigeru Yoshida]

Hi Tung,

On Tue, 16 Jul 2024 07:35:50 +0000, Tung Nguyen wrote:
>>tipc_udp_addr2str() should return non-zero value if the UDP media address is invalid. Otherwise, a buffer overflow access can occur in
>>tipc_media_addr_printf(). Fix this by returning 1 on an invalid UDP media address.
>>
>>Fixes: d0f91938bede ("tipc: add ip/udp media type")
>>Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>>---
>> net/tipc/udp_media.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>>diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index b849a3d133a0..439f75539977 100644
>>--- a/net/tipc/udp_media.c
>>+++ b/net/tipc/udp_media.c
>>@@ -135,8 +135,11 @@ static int tipc_udp_addr2str(struct tipc_media_addr *a, char *buf, int size)
>>                snprintf(buf, size, "%pI4:%u", &ua->ipv4, ntohs(ua->port));
>>        else if (ntohs(ua->proto) == ETH_P_IPV6)
>>                snprintf(buf, size, "%pI6:%u", &ua->ipv6, ntohs(ua->port));
>>-       else
>>+       else {
>>                pr_err("Invalid UDP media address\n");
>>+               return 1;
> Please use -EINVAL instead.

Other addr2str functions like tipc_eth_addr2str() use 1, so I followed
convention. But -EINVAL is more appropriate, as you say.

Thanks,
Shigeru

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Paolo Abeni]

On 7/16/24 09:45, Shigeru Yoshida wrote:
> On Tue, 16 Jul 2024 07:35:50 +0000, Tung Nguyen wrote:
>>> net/tipc/udp_media.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index b849a3d133a0..439f75539977 100644
>>> --- a/net/tipc/udp_media.c
>>> +++ b/net/tipc/udp_media.c
>>> @@ -135,8 +135,11 @@ static int tipc_udp_addr2str(struct tipc_media_addr *a, char *buf, int size)
>>>                 snprintf(buf, size, "%pI4:%u", &ua->ipv4, ntohs(ua->port));
>>>         else if (ntohs(ua->proto) == ETH_P_IPV6)
>>>                 snprintf(buf, size, "%pI6:%u", &ua->ipv6, ntohs(ua->port));
>>> -       else
>>> +       else {
>>>                 pr_err("Invalid UDP media address\n");
>>> +               return 1;
>> Please use -EINVAL instead.
> 
> Other addr2str functions like tipc_eth_addr2str() use 1, so I followed
> convention. But -EINVAL is more appropriate, as you say.

I think that consistency with other tipc helpers here would be more 
appropriate: IMHO no need to send a v2.

@Tung: please trim your replies to only include the relevant quoted text 
and fix your configuration to avoid inserting the long trailer, quite 
unsuitable for messages sent to a public ML.

Thanks,

Paolo

RE: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Tung Nguyen]

>I think that consistency with other tipc helpers here would be more
>appropriate: IMHO no need to send a v2.
>
I do not think so. If you look at other helper functions for udp media, they use predefined error codes, for example:
tipc_udp_msg2addr()
{
 ...
return -EINVAL;
 ...
}

Helper functions for ethernet media should use predefined macros as well. So, they really need a "clean-up" patch later for net-next.

>@Tung: please trim your replies to only include the relevant quoted text and fix your configuration to avoid inserting the long trailer,
>quite unsuitable for messages sent to a public ML.
>
I am aware of that and tried to fix but it seems out of my control for now. Please give me some more time to understand what's wrong with the new mail server. (It was no issue with my old email dektech.com.au)

The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions.

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Paolo Abeni]

On 7/16/24 13:43, Tung Nguyen wrote:
> 
>> I think that consistency with other tipc helpers here would be more
>> appropriate: IMHO no need to send a v2.
>>
> I do not think so. If you look at other helper functions for udp media, they use predefined error codes, for example:
> tipc_udp_msg2addr()
> {
>   ...
> return -EINVAL;
>   ...
> }

It's not a big deal really, but, as noted by Shigeru, all the other 
tipc_*_addr2str() callbacks return 1 on error and such callback is 
invoked via function pointer.

If only this one returns a negative error, modification to the function 
pointer callsite will become prone to errors (and stable backports more 
fragiles)

Cheers,

Paolo

RE: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Tung Nguyen]

>If only this one returns a negative error, modification to the function pointer callsite will become prone to errors (and stable backports
>more
>fragiles)
>
I really do not see any issue with returning a negative error which is the correct thing to do. The function pointer call returns 0 on success, non-zero on error as expected.
I do not see "prone-to-error" when it comes to backport.
As said, problem is returning 1 in infiniband and ethernet media that should be corrected.

The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions.

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Shigeru Yoshida]

Hi Tung, Paolo,

On Tue, 16 Jul 2024 13:29:08 +0000, Tung Nguyen wrote:
>>If only this one returns a negative error, modification to the function pointer callsite will become prone to errors (and stable backports
>>more
>>fragiles)
>>
> I really do not see any issue with returning a negative error which is the correct thing to do. The function pointer call returns 0 on success, non-zero on error as expected.
> I do not see "prone-to-error" when it comes to backport.
> As said, problem is returning 1 in infiniband and ethernet media that should be corrected.

Thank you for your comments.

I understand Tung's point. Returning 1 is not descriptive. But I think
addr2str() functions need consistency for the return value,
i.e. return 1 on error.

How about merging this patch for bug fix and consistency, and then
submitting a cleanup patch for returning -EINVAL on all addr2str()
functions?

Thanks,
Shigeru

RE: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Tung Nguyen]

>How about merging this patch for bug fix and consistency, and then submitting a cleanup patch for returning -EINVAL on all addr2str()
>functions?
>
I agree with this proposal.

Reviewed-by: Tung Nguyen <tung.q.nguyen@endava.com>

The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions.

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Shigeru Yoshida]

On Wed, 17 Jul 2024 02:10:33 +0000, Tung Nguyen wrote:
>>How about merging this patch for bug fix and consistency, and then submitting a cleanup patch for returning -EINVAL on all addr2str()
>>functions?
>>
> I agree with this proposal.
> 
> Reviewed-by: Tung Nguyen <tung.q.nguyen@endava.com>

Thanks!!

Shigeru

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Jakub Kicinski]

On Wed, 17 Jul 2024 02:10:33 +0000 Tung Nguyen wrote:
> I agree with this proposal.
> 
> Reviewed-by: Tung Nguyen <tung.q.nguyen@endava.com>
> 
> The information in this email is confidential and may be legally privileged. ...

What do you expect us to do with a review tag that has a two-paragraph
legal license attached to it?

RE: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Tung Nguyen]

>> Reviewed-by: Tung Nguyen <tung.q.nguyen@endava.com>
>>
>> The information in this email is confidential and may be legally privileged. ...
>
>What do you expect us to do with a review tag that has a two-paragraph legal license attached to it?
Please ignore that disclaimer message. I am still asking help from my organization to remove that annoying message.

The information in this email is confidential and may be legally privileged. It is intended solely for the addressee. Any opinions expressed are mine and do not necessarily represent the opinions of the Company. Emails are susceptible to interference. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is strictly prohibited and may be unlawful. If you have received this message in error, do not open any attachments but please notify the Endava Service Desk on (+44 (0)870 423 0187), and delete this message from your system. The sender accepts no responsibility for information, errors or omissions in this email, or for its use or misuse, or for any act committed or omitted in connection with this communication. If in doubt, please verify the authenticity of the contents with the sender. Please rely on your own virus checkers as no responsibility is taken by the sender for any damage rising out of any bug or virus infection.

Endava plc is a company registered in England under company number 5722669 whose registered office is at 125 Old Broad Street, London, EC2N 1AR, United Kingdom. Endava plc is the Endava group holding company and does not provide any services to clients. Each of Endava plc and its subsidiaries is a separate legal entity and has no liability for another such entity's acts or omissions.

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[Paolo Abeni]

On 7/18/24 03:30, Tung Nguyen wrote:
>>> Reviewed-by: Tung Nguyen <tung.q.nguyen@endava.com>
>>>
>>> The information in this email is confidential and may be legally privileged. ...
>>
>> What do you expect us to do with a review tag that has a two-paragraph legal license attached to it?
> Please ignore that disclaimer message. I am still asking help from my organization to remove that annoying message.

The unfortunate thing is that the message has legal implication which do 
not fit well with formal tags. If you can't trim the trailing message 
from your corporate account - I know it could be even very hard or 
impossible - please use a different account that you can control more 
easily for formal interaction on the ML.

Thanks,

Paolo

Re: [PATCH net] tipc: Return non-zero value from tipc_udp_addr2str() on error

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 16 Jul 2024 11:09:05 +0900 you wrote:
> tipc_udp_addr2str() should return non-zero value if the UDP media
> address is invalid. Otherwise, a buffer overflow access can occur in
> tipc_media_addr_printf(). Fix this by returning 1 on an invalid UDP
> media address.
> 
> Fixes: d0f91938bede ("tipc: add ip/udp media type")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> 
> [...]

Here is the summary with links:
  - [net] tipc: Return non-zero value from tipc_udp_addr2str() on error
    https://git.kernel.org/netdev/net/c/fa96c6baef1b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html