[PATCH] rxrpc/proc: size address buffers for %pISpc output

[PATCH] rxrpc/proc: size address buffers for %pISpc output

[Pengpeng Hou]

The AF_RXRPC procfs helpers format local and remote socket addresses into
fixed 50-byte stack buffers with "%pISpc".

That is too small for the longest IPv6-with-port form the formatter can
produce: a compressed IPv6 address still permits a bracketed mapped-IPv4
spelling plus the trailing port, which exceeds 50 bytes including the final
NUL.

Size the buffers from the formatters maximum textual form and switch the
call sites to scnprintf().

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 net/rxrpc/proc.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
index 59292f7f9205..7925d4569776 100644
--- a/net/rxrpc/proc.c
+++ b/net/rxrpc/proc.c
@@ -10,6 +10,10 @@
 #include <net/af_rxrpc.h>
 #include "ar-internal.h"
 
+#define RXRPC_PROC_ADDRBUF_SIZE \
+	(sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") + \
+	 sizeof(":12345"))
+
 static const char *const rxrpc_conn_states[RXRPC_CONN__NR_STATES] = {
 	[RXRPC_CONN_UNUSED]			= "Unused  ",
 	[RXRPC_CONN_CLIENT_UNSECURED]		= "ClUnsec ",
@@ -53,7 +57,7 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
 	enum rxrpc_call_state state;
 	rxrpc_seq_t tx_bottom;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 	long timeout = 0;
 
 	if (v == &rxnet->calls) {
@@ -69,11 +73,11 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
 
 	local = call->local;
 	if (local)
-		sprintf(lbuff, "%pISpc", &local->srx.transport);
+		scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
 	else
 		strcpy(lbuff, "no_local");
 
-	sprintf(rbuff, "%pISpc", &call->dest_srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &call->dest_srx.transport);
 
 	state = rxrpc_call_state(call);
 	if (state != RXRPC_CALL_SERVER_PREALLOC)
@@ -142,7 +146,7 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
 	struct rxrpc_connection *conn;
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
 	const char *state;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == &rxnet->conn_proc_list) {
 		seq_puts(seq,
@@ -161,8 +165,8 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
 		goto print;
 	}
 
-	sprintf(lbuff, "%pISpc", &conn->local->srx.transport);
-	sprintf(rbuff, "%pISpc", &conn->peer->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &conn->local->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &conn->peer->srx.transport);
 print:
 	state = rxrpc_is_conn_aborted(conn) ?
 		rxrpc_call_completions[conn->completion] :
@@ -228,7 +232,7 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_bundle *bundle;
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == &rxnet->bundle_proc_list) {
 		seq_puts(seq,
@@ -242,8 +246,8 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
 
 	bundle = list_entry(v, struct rxrpc_bundle, proc_link);
 
-	sprintf(lbuff, "%pISpc", &bundle->local->srx.transport);
-	sprintf(rbuff, "%pISpc", &bundle->peer->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &bundle->local->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &bundle->peer->srx.transport);
 	seq_printf(seq,
 		   "UDP   %-47.47s %-47.47s %4x %3u %3d"
 		   " %c%c%c %08x | %08x %08x %08x %08x %08x\n",
@@ -279,7 +283,7 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_peer *peer;
 	time64_t now;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == SEQ_START_TOKEN) {
 		seq_puts(seq,
@@ -290,9 +294,9 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
 
 	peer = list_entry(v, struct rxrpc_peer, hash_link);
 
-	sprintf(lbuff, "%pISpc", &peer->local->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &peer->local->srx.transport);
 
-	sprintf(rbuff, "%pISpc", &peer->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &peer->srx.transport);
 
 	now = ktime_get_seconds();
 	seq_printf(seq,
@@ -401,7 +405,7 @@ const struct seq_operations rxrpc_peer_seq_ops = {
 static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_local *local;
-	char lbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == SEQ_START_TOKEN) {
 		seq_puts(seq,
@@ -412,7 +416,7 @@ static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
 
 	local = hlist_entry(v, struct rxrpc_local, link);
 
-	sprintf(lbuff, "%pISpc", &local->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
 
 	seq_printf(seq,
 		   "UDP   %-47.47s %3u %3u %3u\n",
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] rxrpc/proc: size address buffers for %pISpc output

[Anderson Nascimento]

On 4/4/26 6:32 AM, Pengpeng Hou wrote:
> The AF_RXRPC procfs helpers format local and remote socket addresses into
> fixed 50-byte stack buffers with "%pISpc".
>
> That is too small for the longest IPv6-with-port form the formatter can
> produce: a compressed IPv6 address still permits a bracketed mapped-IPv4
> spelling plus the trailing port, which exceeds 50 bytes including the final
> NUL.
>
> Size the buffers from the formatters maximum textual form and switch the
> call sites to scnprintf().
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>   net/rxrpc/proc.c | 32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
> index 59292f7f9205..7925d4569776 100644
> --- a/net/rxrpc/proc.c
> +++ b/net/rxrpc/proc.c
> @@ -10,6 +10,10 @@
>   #include <net/af_rxrpc.h>
>   #include "ar-internal.h"
>   
> +#define RXRPC_PROC_ADDRBUF_SIZE \
> +	(sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") + \
> +	 sizeof(":12345"))
> +
>   static const char *const rxrpc_conn_states[RXRPC_CONN__NR_STATES] = {
>   	[RXRPC_CONN_UNUSED]			= "Unused  ",
>   	[RXRPC_CONN_CLIENT_UNSECURED]		= "ClUnsec ",
> @@ -53,7 +57,7 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
>   	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
>   	enum rxrpc_call_state state;
>   	rxrpc_seq_t tx_bottom;
> -	char lbuff[50], rbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
>   	long timeout = 0;
>   
>   	if (v == &rxnet->calls) {
> @@ -69,11 +73,11 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
>   
>   	local = call->local;
>   	if (local)
> -		sprintf(lbuff, "%pISpc", &local->srx.transport);
> +		scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
>   	else
>   		strcpy(lbuff, "no_local");
>   
> -	sprintf(rbuff, "%pISpc", &call->dest_srx.transport);
> +	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &call->dest_srx.transport);
>   
>   	state = rxrpc_call_state(call);
>   	if (state != RXRPC_CALL_SERVER_PREALLOC)
> @@ -142,7 +146,7 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
>   	struct rxrpc_connection *conn;
>   	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
>   	const char *state;
> -	char lbuff[50], rbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
>   
>   	if (v == &rxnet->conn_proc_list) {
>   		seq_puts(seq,
> @@ -161,8 +165,8 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
>   		goto print;
>   	}
>   
> -	sprintf(lbuff, "%pISpc", &conn->local->srx.transport);
> -	sprintf(rbuff, "%pISpc", &conn->peer->srx.transport);
> +	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &conn->local->srx.transport);
> +	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &conn->peer->srx.transport);
>   print:
>   	state = rxrpc_is_conn_aborted(conn) ?
>   		rxrpc_call_completions[conn->completion] :
> @@ -228,7 +232,7 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
>   {
>   	struct rxrpc_bundle *bundle;
>   	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
> -	char lbuff[50], rbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
>   
>   	if (v == &rxnet->bundle_proc_list) {
>   		seq_puts(seq,
> @@ -242,8 +246,8 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
>   
>   	bundle = list_entry(v, struct rxrpc_bundle, proc_link);
>   
> -	sprintf(lbuff, "%pISpc", &bundle->local->srx.transport);
> -	sprintf(rbuff, "%pISpc", &bundle->peer->srx.transport);
> +	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &bundle->local->srx.transport);
> +	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &bundle->peer->srx.transport);
>   	seq_printf(seq,
>   		   "UDP   %-47.47s %-47.47s %4x %3u %3d"
>   		   " %c%c%c %08x | %08x %08x %08x %08x %08x\n",
> @@ -279,7 +283,7 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
>   {
>   	struct rxrpc_peer *peer;
>   	time64_t now;
> -	char lbuff[50], rbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
>   
>   	if (v == SEQ_START_TOKEN) {
>   		seq_puts(seq,
> @@ -290,9 +294,9 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
>   
>   	peer = list_entry(v, struct rxrpc_peer, hash_link);
>   
> -	sprintf(lbuff, "%pISpc", &peer->local->srx.transport);
> +	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &peer->local->srx.transport);
>   
> -	sprintf(rbuff, "%pISpc", &peer->srx.transport);
> +	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &peer->srx.transport);
>   
>   	now = ktime_get_seconds();
>   	seq_printf(seq,
> @@ -401,7 +405,7 @@ const struct seq_operations rxrpc_peer_seq_ops = {
>   static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
>   {
>   	struct rxrpc_local *local;
> -	char lbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE];
>   
>   	if (v == SEQ_START_TOKEN) {
>   		seq_puts(seq,
> @@ -412,7 +416,7 @@ static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
>   
>   	local = hlist_entry(v, struct rxrpc_local, link);
>   
> -	sprintf(lbuff, "%pISpc", &local->srx.transport);
> +	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
>   
>   	seq_printf(seq,
>   		   "UDP   %-47.47s %3u %3u %3u\n",

Do you have any evidence to confirm this issue? While the code could be 
improved, I don't see how that specific error would occur. Based on a 
quick experiment, the resulting string is at most 47 or 48 bytes (e.g., 
|[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]:65535|).

Re: [PATCH] rxrpc/proc: size address buffers for %pISpc output

[Anderson Nascimento]

On 4/6/26 3:10 AM, Pengpeng Hou wrote:
> Hi,
>
> Yes. My original changelog example was too loose, and your quick test is
> right for a fully expanded plain IPv6 form such as
>
>    [ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]:65535
>
> That form is only 47 visible characters, so it fits in the current
> char[50] buffers.
>
> The reason I still think the bug is real is the current %pISpc
> implementation in lib/vsprintf.c.
>
> For AF_INET6, %pISpc goes through ip6_addr_string_sa(), and the compressed
> path uses ip6_compressed_string(). That helper switches to a dotted-quad
> tail not only for v4mapped addresses, but also for ISATAP addresses:
>
>    useIPv4 = ipv6_addr_v4mapped(&in6) || ipv6_addr_is_isatap(&in6);
>
> So a current-tree case such as
>
>    [ffff:ffff:ffff:ffff:0:5efe:255.255.255.255]:65535
>
> is possible. That string is 50 visible characters, i.e. 51 bytes
> including the trailing NUL, which does not fit in the existing char[50]
> buffers used by the rxrpc procfs helpers.
>
> So I agree the example in my changelog should be corrected, but I do not
> think the underlying bug goes away. The claim should be framed around the
> ISATAP case rather than the plain IPv6 or mapped-v4 examples I used
> originally.
>
> If that makes sense, I can resend with the changelog corrected to cite the
> actual maximum case explicitly.
>
> Thanks,
> Pengpeng

Thanks! I confirm the ISATAP case really triggers the issue. It ends up 
writing 51 bytes, including the NULL byte.

$ cat /proc/net/rxrpc/locals
Proto Local                                           Use Act RxQ
UDP   [ffff:ffff:ffff:ffff:0:5efe:255.255.255.255]:65   1   1   0
$

Regards,

Anderson Nascimento

Re: [PATCH] rxrpc/proc: size address buffers for %pISpc output

[Pengpeng Hou]

Hi,

Yes. My original changelog example was too loose, and your quick test is
right for a fully expanded plain IPv6 form such as

  [ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]:65535

That form is only 47 visible characters, so it fits in the current
char[50] buffers.

The reason I still think the bug is real is the current %pISpc
implementation in lib/vsprintf.c.

For AF_INET6, %pISpc goes through ip6_addr_string_sa(), and the compressed
path uses ip6_compressed_string(). That helper switches to a dotted-quad
tail not only for v4mapped addresses, but also for ISATAP addresses:

  useIPv4 = ipv6_addr_v4mapped(&in6) || ipv6_addr_is_isatap(&in6);

So a current-tree case such as

  [ffff:ffff:ffff:ffff:0:5efe:255.255.255.255]:65535

is possible. That string is 50 visible characters, i.e. 51 bytes
including the trailing NUL, which does not fit in the existing char[50]
buffers used by the rxrpc procfs helpers.

So I agree the example in my changelog should be corrected, but I do not
think the underlying bug goes away. The claim should be framed around the
ISATAP case rather than the plain IPv6 or mapped-v4 examples I used
originally.

If that makes sense, I can resend with the changelog corrected to cite the
actual maximum case explicitly.

Thanks,
Pengpeng

[PATCH v2] rxrpc/proc: size address buffers for %pISpc output

[Pengpeng Hou]

The AF_RXRPC procfs helpers format local and remote socket addresses into
fixed 50-byte stack buffers with "%pISpc".

That is too small for the longest current-tree IPv6-with-port form the
formatter can produce. In lib/vsprintf.c, the compressed IPv6 path uses a
dotted-quad tail not only for v4mapped addresses, but also for ISATAP
addresses via ipv6_addr_is_isatap().

As a result, a case such as

  [ffff:ffff:ffff:ffff:0:5efe:255.255.255.255]:65535

is possible with the current formatter. That is 50 visible characters, so
51 bytes including the trailing NUL, which does not fit in the existing
char[50] buffers used by net/rxrpc/proc.c.

Size the buffers from the formatter's maximum textual form and switch the
call sites to scnprintf().

Changes since v1:
- correct the changelog to cite the actual maximum current-tree case
  explicitly
- frame the proof around the ISATAP formatting path instead of the earlier
  mapped-v4 example

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 net/rxrpc/proc.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/rxrpc/proc.c b/net/rxrpc/proc.c
index 59292f7f9205..7925d4569776 100644
--- a/net/rxrpc/proc.c
+++ b/net/rxrpc/proc.c
@@ -10,6 +10,10 @@
 #include <net/af_rxrpc.h>
 #include "ar-internal.h"
 
+#define RXRPC_PROC_ADDRBUF_SIZE \
+	(sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") + \
+	 sizeof(":12345"))
+
 static const char *const rxrpc_conn_states[RXRPC_CONN__NR_STATES] = {
 	[RXRPC_CONN_UNUSED]			= "Unused  ",
 	[RXRPC_CONN_CLIENT_UNSECURED]		= "ClUnsec ",
@@ -53,7 +57,7 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
 	enum rxrpc_call_state state;
 	rxrpc_seq_t tx_bottom;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 	long timeout = 0;
 
 	if (v == &rxnet->calls) {
@@ -69,11 +73,11 @@ static int rxrpc_call_seq_show(struct seq_file *seq, void *v)
 
 	local = call->local;
 	if (local)
-		sprintf(lbuff, "%pISpc", &local->srx.transport);
+		scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
 	else
 		strcpy(lbuff, "no_local");
 
-	sprintf(rbuff, "%pISpc", &call->dest_srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &call->dest_srx.transport);
 
 	state = rxrpc_call_state(call);
 	if (state != RXRPC_CALL_SERVER_PREALLOC)
@@ -142,7 +146,7 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
 	struct rxrpc_connection *conn;
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
 	const char *state;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == &rxnet->conn_proc_list) {
 		seq_puts(seq,
@@ -161,8 +165,8 @@ static int rxrpc_connection_seq_show(struct seq_file *seq, void *v)
 		goto print;
 	}
 
-	sprintf(lbuff, "%pISpc", &conn->local->srx.transport);
-	sprintf(rbuff, "%pISpc", &conn->peer->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &conn->local->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &conn->peer->srx.transport);
 print:
 	state = rxrpc_is_conn_aborted(conn) ?
 		rxrpc_call_completions[conn->completion] :
@@ -228,7 +232,7 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_bundle *bundle;
 	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == &rxnet->bundle_proc_list) {
 		seq_puts(seq,
@@ -242,8 +246,8 @@ static int rxrpc_bundle_seq_show(struct seq_file *seq, void *v)
 
 	bundle = list_entry(v, struct rxrpc_bundle, proc_link);
 
-	sprintf(lbuff, "%pISpc", &bundle->local->srx.transport);
-	sprintf(rbuff, "%pISpc", &bundle->peer->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &bundle->local->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &bundle->peer->srx.transport);
 	seq_printf(seq,
 		   "UDP   %-47.47s %-47.47s %4x %3u %3d"
 		   " %c%c%c %08x | %08x %08x %08x %08x %08x\n",
@@ -279,7 +283,7 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_peer *peer;
 	time64_t now;
-	char lbuff[50], rbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == SEQ_START_TOKEN) {
 		seq_puts(seq,
@@ -290,9 +294,9 @@ static int rxrpc_peer_seq_show(struct seq_file *seq, void *v)
 
 	peer = list_entry(v, struct rxrpc_peer, hash_link);
 
-	sprintf(lbuff, "%pISpc", &peer->local->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &peer->local->srx.transport);
 
-	sprintf(rbuff, "%pISpc", &peer->srx.transport);
+	scnprintf(rbuff, sizeof(rbuff), "%pISpc", &peer->srx.transport);
 
 	now = ktime_get_seconds();
 	seq_printf(seq,
@@ -401,7 +405,7 @@ const struct seq_operations rxrpc_peer_seq_ops = {
 static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
 {
 	struct rxrpc_local *local;
-	char lbuff[50];
+	char lbuff[RXRPC_PROC_ADDRBUF_SIZE];
 
 	if (v == SEQ_START_TOKEN) {
 		seq_puts(seq,
@@ -412,7 +416,7 @@ static int rxrpc_local_seq_show(struct seq_file *seq, void *v)
 
 	local = hlist_entry(v, struct rxrpc_local, link);
 
-	sprintf(lbuff, "%pISpc", &local->srx.transport);
+	scnprintf(lbuff, sizeof(lbuff), "%pISpc", &local->srx.transport);
 
 	seq_printf(seq,
 		   "UDP   %-47.47s %3u %3u %3u\n",
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] rxrpc/proc: size address buffers for %pISpc output

[Pengpeng Hou]

Hi,

Thanks for confirming that.

That is very helpful. It matches the current-tree ISATAP case in
lib/vsprintf.c and confirms that the formatter ends up writing 51 bytes
including the trailing NUL into the existing 50-byte stack buffer.

I'll resend the patch with the changelog corrected to frame the proof
around the ISATAP case explicitly.

Thanks,
Pengpeng

Re: [PATCH v2] rxrpc/proc: size address buffers for %pISpc output

[David Howells]

Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> -	char lbuff[50], rbuff[50];
> +	char lbuff[RXRPC_PROC_ADDRBUF_SIZE], rbuff[RXRPC_PROC_ADDRBUF_SIZE];

This might be saved upstream by the compiler padding the buffers out to
sizeof(long) because they're on the stack.

David

Re: [PATCH v2] rxrpc/proc: size address buffers for %pISpc output

[Pengpeng Hou]

Hi David,

That's possible for some builds, yes.

My concern here is a bit narrower: with the current %pISpc formatter, the
ISATAP case can produce 50 visible characters, i.e. 51 bytes including
the trailing NUL, while these helpers pass a declared char[50] object to
sprintf().

So even if a particular compiler/ABI happens to pad the stack slot out to
sizeof(long) or more, that would only be incidental code generation slack.
It still writes one byte past the end of the declared object, and whether
that lands in padding or in another live stack slot depends on the build
and the specific function layout.

In particular, rxrpc_local_seq_show() has only a single local char[50]
buffer, so there is no source-level guarantee of spare space there either.

The patch is really just making the object size match the formatter's
actual maximum output, so we don't depend on incidental stack padding.

Thanks,
Pengpeng