[PATCH net] tcp: switch snprintf to scnprintf

[PATCH net] tcp: switch snprintf to scnprintf

[Hangbin Liu]

snprintf returns the number of chars that would be written, not number
of chars that were actually written. As such, 'offs' may get larger than
'tbl.maxlen', causing the 'tbl.maxlen - offs' being < 0, and since the
parameter is size_t, it would overflow.

Currently, the buffer is still enough, but for future design, use scnprintf
would be safer.

Suggested-by: Jiri Benc <jbenc@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/sysctl_net_ipv4.c | 14 +++++++-------
 net/ipv4/tcp_cong.c        | 12 ++++++------
 net/ipv4/tcp_ulp.c         |  6 +++---
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 59ded25acd04..ed804cf68026 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -334,14 +334,14 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
 		user_key[i] = le32_to_cpu(key[i]);
 
 	for (i = 0; i < n_keys; i++) {
-		off += snprintf(tbl.data + off, tbl.maxlen - off,
-				"%08x-%08x-%08x-%08x",
-				user_key[i * 4],
-				user_key[i * 4 + 1],
-				user_key[i * 4 + 2],
-				user_key[i * 4 + 3]);
+		off += scnprintf(tbl.data + off, tbl.maxlen - off,
+				 "%08x-%08x-%08x-%08x",
+				 user_key[i * 4],
+				 user_key[i * 4 + 1],
+				 user_key[i * 4 + 2],
+				 user_key[i * 4 + 3]);
 		if (i + 1 < n_keys)
-			off += snprintf(tbl.data + off, tbl.maxlen - off, ",");
+			off += scnprintf(tbl.data + off, tbl.maxlen - off, ",");
 	}
 
 	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index c445a81d144e..d84a09bd0201 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -253,9 +253,9 @@ void tcp_get_available_congestion_control(char *buf, size_t maxlen)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(ca, &tcp_cong_list, list) {
-		offs += snprintf(buf + offs, maxlen - offs,
-				 "%s%s",
-				 offs == 0 ? "" : " ", ca->name);
+		offs += scnprintf(buf + offs, maxlen - offs,
+				  "%s%s",
+				  offs == 0 ? "" : " ", ca->name);
 	}
 	rcu_read_unlock();
 }
@@ -282,9 +282,9 @@ void tcp_get_allowed_congestion_control(char *buf, size_t maxlen)
 	list_for_each_entry_rcu(ca, &tcp_cong_list, list) {
 		if (!(ca->flags & TCP_CONG_NON_RESTRICTED))
 			continue;
-		offs += snprintf(buf + offs, maxlen - offs,
-				 "%s%s",
-				 offs == 0 ? "" : " ", ca->name);
+		offs += scnprintf(buf + offs, maxlen - offs,
+				  "%s%s",
+				  offs == 0 ? "" : " ", ca->name);
 	}
 	rcu_read_unlock();
 }
diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
index 4849edb62d52..c338cae13842 100644
--- a/net/ipv4/tcp_ulp.c
+++ b/net/ipv4/tcp_ulp.c
@@ -89,9 +89,9 @@ void tcp_get_available_ulp(char *buf, size_t maxlen)
 	*buf = '\0';
 	rcu_read_lock();
 	list_for_each_entry_rcu(ulp_ops, &tcp_ulp_list, list) {
-		offs += snprintf(buf + offs, maxlen - offs,
-				 "%s%s",
-				 offs == 0 ? "" : " ", ulp_ops->name);
+		offs += scnprintf(buf + offs, maxlen - offs,
+				  "%s%s",
+				  offs == 0 ? "" : " ", ulp_ops->name);
 	}
 	rcu_read_unlock();
 }
-- 
2.19.2

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Eric Dumazet]

On 11/14/19 2:28 AM, Hangbin Liu wrote:
> snprintf returns the number of chars that would be written, not number
> of chars that were actually written. As such, 'offs' may get larger than
> 'tbl.maxlen', causing the 'tbl.maxlen - offs' being < 0, and since the
> parameter is size_t, it would overflow.
> 
> Currently, the buffer is still enough, but for future design, use scnprintf
> would be safer.
>

Why is it targeting net tree ?

How have you checked that it was actually safer ?

This looks unnecessary code churn to me, and it might hide an error in the future.

We need to properly size the output buffers before using them,
we can not afford truncating silently the output.

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Hangbin Liu]

Hi Eric,
On Thu, Nov 14, 2019 at 06:28:35AM -0800, Eric Dumazet wrote:
> 
> 
> On 11/14/19 2:28 AM, Hangbin Liu wrote:
> > snprintf returns the number of chars that would be written, not number
> > of chars that were actually written. As such, 'offs' may get larger than
> > 'tbl.maxlen', causing the 'tbl.maxlen - offs' being < 0, and since the
> > parameter is size_t, it would overflow.
> > 
> > Currently, the buffer is still enough, but for future design, use scnprintf
> > would be safer.
> >
> 
> Why is it targeting net tree ?

I though this is a small fixup. Maybe my though is not rigor enough. I don't
have much intend to net or net-next.
> 
> How have you checked that it was actually safer ?

No, this patch just from code review. I only did a test in user space
that snprintf could cause overflow. But I found some similar fixes
in kernel.

63350bdb3845 staging: vhciq_core: replace snprintf with scnprintf
37e444c8296c usb: Replace snprintf with scnprintf in gether_get_ifname
bd17cc5a20ae test_firmware: Use correct snprintf() limit
e7f7b6f38a44 scsi: lpfc: change snprintf to scnprintf for possible overflow

> 
> This looks unnecessary code churn to me, and it might hide an error in the future.

Not sure if I understand correctly, do you mean we may rely scnprintf
to much, and not set size correctly and got truncated message?
> 
> We need to properly size the output buffers before using them,
> we can not afford truncating silently the output.
> 

Yes, I agree. Just as I said, the buffer is still enough, while scnprintf
is just a safer usage compired with snprintf.

Thanks
Hangbin

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Jiri Benc]

On Fri, 15 Nov 2019 10:41:06 +0800, Hangbin Liu wrote:
> > We need to properly size the output buffers before using them,
> > we can not afford truncating silently the output.
> 
> Yes, I agree. Just as I said, the buffer is still enough, while scnprintf
> is just a safer usage compired with snprintf.

So maybe keep snprintf but add WARN_ON and bail out of the loop if the
buffer size was reached?

	if (WARN_ON(offs >= maxlen))
		break;

 Jiri

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Hangbin Liu]

On Fri, Nov 15, 2019 at 10:54:55AM +0100, Jiri Benc wrote:
> On Fri, 15 Nov 2019 10:41:06 +0800, Hangbin Liu wrote:
> > > We need to properly size the output buffers before using them,
> > > we can not afford truncating silently the output.
> > 
> > Yes, I agree. Just as I said, the buffer is still enough, while scnprintf
> > is just a safer usage compired with snprintf.
> 
> So maybe keep snprintf but add WARN_ON and bail out of the loop if the
> buffer size was reached?
> 
> 	if (WARN_ON(offs >= maxlen))
> 		break;
> 

Hi Eric,

What do you think?

Thanks
Hangbin

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Eric Dumazet]

On 11/18/19 5:53 PM, Hangbin Liu wrote:
> On Fri, Nov 15, 2019 at 10:54:55AM +0100, Jiri Benc wrote:
>> On Fri, 15 Nov 2019 10:41:06 +0800, Hangbin Liu wrote:
>>>> We need to properly size the output buffers before using them,
>>>> we can not afford truncating silently the output.
>>>
>>> Yes, I agree. Just as I said, the buffer is still enough, while scnprintf
>>> is just a safer usage compired with snprintf.
>>
>> So maybe keep snprintf but add WARN_ON and bail out of the loop if the
>> buffer size was reached?
>>
>> 	if (WARN_ON(offs >= maxlen))
>> 		break;
>>
> 
> Hi Eric,
> 
> What do you think?
> 

WARN_ON_ONCE() please...

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Hangbin Liu]

On Mon, Nov 18, 2019 at 07:48:32PM -0800, Eric Dumazet wrote:
> 
> 
> On 11/18/19 5:53 PM, Hangbin Liu wrote:
> > On Fri, Nov 15, 2019 at 10:54:55AM +0100, Jiri Benc wrote:
> >> On Fri, 15 Nov 2019 10:41:06 +0800, Hangbin Liu wrote:
> >>>> We need to properly size the output buffers before using them,
> >>>> we can not afford truncating silently the output.
> >>>
> >>> Yes, I agree. Just as I said, the buffer is still enough, while scnprintf
> >>> is just a safer usage compired with snprintf.
> >>
> >> So maybe keep snprintf but add WARN_ON and bail out of the loop if the
> >> buffer size was reached?
> >>
> >> 	if (WARN_ON(offs >= maxlen))
> >> 		break;
> >>
> > 
> > Hi Eric,
> > 
> > What do you think?
> > 
> 
> WARN_ON_ONCE() please...
> 
OK, I will post a v2 update. Should it target to net or net-next?

Thanks
Hangbin

Re: [PATCH net] tcp: switch snprintf to scnprintf

[Eric Dumazet]

On 11/19/19 5:40 AM, Hangbin Liu wrote:

>>
> OK, I will post a v2 update. Should it target to net or net-next?
> 

Since there is no bug to fix, net-next is appropriate.

[PATCHv2 net-next] tcp: warn if offset reach the maxlen limit when using snprintf

[Hangbin Liu]

snprintf returns the number of chars that would be written, not number
of chars that were actually written. As such, 'offs' may get larger than
'tbl.maxlen', causing the 'tbl.maxlen - offs' being < 0, and since the
parameter is size_t, it would overflow.

Since using scnprintf may hide the limit error, while the buffer is still
enough now, let's just add a WARN_ON_ONCE in case it reach the limit
in future.

v2: Use WARN_ON_ONCE as Jiri and Eric suggested.

Suggested-by: Jiri Benc <jbenc@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv4/sysctl_net_ipv4.c | 4 ++++
 net/ipv4/tcp_cong.c        | 6 ++++++
 net/ipv4/tcp_ulp.c         | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 59ded25acd04..c9eaf924df63 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -340,6 +340,10 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
 				user_key[i * 4 + 1],
 				user_key[i * 4 + 2],
 				user_key[i * 4 + 3]);
+
+		if (WARN_ON_ONCE(off >= tbl.maxlen - 1))
+			break;
+
 		if (i + 1 < n_keys)
 			off += snprintf(tbl.data + off, tbl.maxlen - off, ",");
 	}
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index c445a81d144e..3737ec096650 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -256,6 +256,9 @@ void tcp_get_available_congestion_control(char *buf, size_t maxlen)
 		offs += snprintf(buf + offs, maxlen - offs,
 				 "%s%s",
 				 offs == 0 ? "" : " ", ca->name);
+
+		if (WARN_ON_ONCE(offs >= maxlen))
+			break;
 	}
 	rcu_read_unlock();
 }
@@ -285,6 +288,9 @@ void tcp_get_allowed_congestion_control(char *buf, size_t maxlen)
 		offs += snprintf(buf + offs, maxlen - offs,
 				 "%s%s",
 				 offs == 0 ? "" : " ", ca->name);
+
+		if (WARN_ON_ONCE(offs >= maxlen))
+			break;
 	}
 	rcu_read_unlock();
 }
diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
index 4849edb62d52..12ab5db2b71c 100644
--- a/net/ipv4/tcp_ulp.c
+++ b/net/ipv4/tcp_ulp.c
@@ -92,6 +92,9 @@ void tcp_get_available_ulp(char *buf, size_t maxlen)
 		offs += snprintf(buf + offs, maxlen - offs,
 				 "%s%s",
 				 offs == 0 ? "" : " ", ulp_ops->name);
+
+		if (WARN_ON_ONCE(offs >= maxlen))
+			break;
 	}
 	rcu_read_unlock();
 }
-- 
2.19.2

Re: [PATCHv2 net-next] tcp: warn if offset reach the maxlen limit when using snprintf

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 20 Nov 2019 16:38:08 +0800

> snprintf returns the number of chars that would be written, not number
> of chars that were actually written. As such, 'offs' may get larger than
> 'tbl.maxlen', causing the 'tbl.maxlen - offs' being < 0, and since the
> parameter is size_t, it would overflow.
> 
> Since using scnprintf may hide the limit error, while the buffer is still
> enough now, let's just add a WARN_ON_ONCE in case it reach the limit
> in future.
> 
> v2: Use WARN_ON_ONCE as Jiri and Eric suggested.
> 
> Suggested-by: Jiri Benc <jbenc@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied.