On 2020-11-30 11:04, Dan Carpenter wrote:
From: "kiyin(尹亮)" <kiyin@xxxxxxxxxxx>
The .x25_addr[] address comes from the user and is not necessarily
NUL terminated. This leads to a couple problems. The first problem is
that the strlen() in x25_bind() can read beyond the end of the buffer.
The second problem is more subtle and could result in memory
corruption.
The call tree is:
x25_connect()
--> x25_write_internal()
--> x25_addr_aton()
The .x25_addr[] buffers are copied to the "addresses" buffer from
x25_write_internal() so it will lead to stack corruption.
The x25 protocol only allows 15 character addresses so putting a NUL
terminator as the 16th character is safe and obviously preferable to
reading out of bounds.
OK, I see the potential danger. I'm just wondering what is the better
approach here to counteract it:
1. check if the string is terminated or exceeds the maximum allowed
length and report an error if necessary.
2. always terminate the string at byte 15 as you suggested.
Signed-off-by: "kiyin(尹亮)" <kiyin@xxxxxxxxxxx>
Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
---
net/x25/af_x25.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 0bbb283f23c9..3180f15942fe 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -686,6 +686,8 @@ static int x25_bind(struct socket *sock, struct
sockaddr *uaddr, int addr_len)
goto out;
}
+ addr->sx25_addr.x25_addr[X25_ADDR_LEN - 1] = '\0';
+
/* check for the null_x25_address */
if (strcmp(addr->sx25_addr.x25_addr, null_x25_address.x25_addr)) {
@@ -779,6 +781,7 @@ static int x25_connect(struct socket *sock, struct
sockaddr *uaddr,
goto out;
rc = -ENETUNREACH;
+ addr->sx25_addr.x25_addr[X25_ADDR_LEN - 1] = '\0';
rt = x25_get_route(&addr->sx25_addr);
if (!rt)
goto out;
