Tea Inside Mailing List <[email protected]>
 help / color / mirror / Atom feed
* [PATCH teavpn2 0/3] teavpn2 fixes
@ 2022-05-27  0:02 Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 1/3] allocator: Fix `@errno` value when overflow Alviro Iskandar Setiawan
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2022-05-27  0:02 UTC (permalink / raw)
  To: Ammar Faizi
  Cc: Alviro Iskandar Setiawan, GNU/Weeb Mailing List,
	Tea Inside Mailing List, Ammar Faizi, Louvian Lyndal,
	Michael Arminto

Hi all,

I have a few TeaVPN2 fixes, please review. 3 patches below:

### Patch 1
The calloc() function from libc sets the @errno variable to ENOMEM
when overflow, not to EOVERFLOW. Change it to ENOMEM to follow libc
error code.

### Patch 2
The malloc() call in escapeshellarg() doesn't have a NULL check. This
results in a potential NULL pointer dereference. Fix this by checking
the return value of malloc(). Just return NULL directly if we hit the
ENOMEM case.

### Patch 3
The libc syscall wrappers mostly return -1 when they fail, then they
set the error code to the @errno variable. The current code seems to
be doing something wrong. We assume it errors when the return value
is negative. However, not all negative values are meant to be an
error indicator. On Linux, the only reserved error code is within
range [-4095, -1]. That means we still have a potential to get a
negative return value that is not an error.

I understand that most of them work fine here because of the nature
of the syscall itself that won't return a negative value upen
succcessful. But the above assumption about the negative value is
not correct.

Replace the error checking from (ret < 0) to (ret == -1) to reflect
the above fact.

Cc: Ammar Faizi <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Cc: Michael Arminto <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>

---
Alviro Iskandar Setiawan (3):
  allocator: Fix `@errno` value when overflow
  net: iface: Fix a potential NULL pointer dereference
  arch/linux: syscall: Fix retval checking in libc syscall

 src/teavpn2/allocator.c          |  2 +-
 src/teavpn2/arch/generic/linux.h | 12 ++++++------
 src/teavpn2/net/linux/iface.c    |  7 +++++--
 3 files changed, 12 insertions(+), 9 deletions(-)


base-commit: 5e5223089d02c6fde68a0b567ca802317be59467
prerequisite-patch-id: b71545410b349281e6ead6ff1dcc1f71f8ab30a4
prerequisite-patch-id: 7fe15b03300490b8aa25cac4dd0be0bd3ce7a4bb
-- 
Alviro Iskandar Setiawan


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH teavpn2 1/3] allocator: Fix `@errno` value when overflow
  2022-05-27  0:02 [PATCH teavpn2 0/3] teavpn2 fixes Alviro Iskandar Setiawan
@ 2022-05-27  0:02 ` Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 2/3] net: iface: Fix a potential NULL pointer dereference Alviro Iskandar Setiawan
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2022-05-27  0:02 UTC (permalink / raw)
  To: Ammar Faizi
  Cc: Alviro Iskandar Setiawan, GNU/Weeb Mailing List,
	Tea Inside Mailing List, Ammar Faizi, Louvian Lyndal,
	Michael Arminto

The calloc() function from libc sets the @errno variable to ENOMEM
when overflow, not to EOVERFLOW. Change it to ENOMEM to follow libc
error code.

Fixes: 44f2ca85b3b923b9405b9da89a1e76039b7bafa1 ("allocator: implement custom allocator to guarantee 64-byte alignment")
Cc: Ammar Faizi <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Cc: Michael Arminto <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
---
 src/teavpn2/allocator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/teavpn2/allocator.c b/src/teavpn2/allocator.c
index 5918009..8aeb2d8 100644
--- a/src/teavpn2/allocator.c
+++ b/src/teavpn2/allocator.c
@@ -32,7 +32,7 @@ noinline void *al64_calloc(size_t nmemb, size_t size)
 	size_t real_size = 0;
 
 	if (unlikely(__builtin_mul_overflow(nmemb, size, &real_size))) {
-		errno = EOVERFLOW;
+		errno = ENOMEM;
 		return NULL;
 	}
 
-- 
Alviro Iskandar Setiawan


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH teavpn2 2/3] net: iface: Fix a potential NULL pointer dereference
  2022-05-27  0:02 [PATCH teavpn2 0/3] teavpn2 fixes Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 1/3] allocator: Fix `@errno` value when overflow Alviro Iskandar Setiawan
@ 2022-05-27  0:02 ` Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 3/3] arch/linux: syscall: Fix retval checking in libc syscall Alviro Iskandar Setiawan
  2022-05-27  0:14 ` [PATCH teavpn2 0/3] teavpn2 fixes Ammar Faizi
  3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2022-05-27  0:02 UTC (permalink / raw)
  To: Ammar Faizi
  Cc: Alviro Iskandar Setiawan, GNU/Weeb Mailing List,
	Tea Inside Mailing List, Ammar Faizi, Louvian Lyndal,
	Michael Arminto

The malloc() call in escapeshellarg() doesn't have a NULL check. This
results in a potential NULL pointer dereference. Fix this by checking
the return value of malloc(). Just return NULL directly if we hit the
ENOMEM case.

Fixes: 0cfd7f8b60a09000a4257015b592e79b0bd8b8bd ("net: rewire iface support for linux")
Cc: Ammar Faizi <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Cc: Michael Arminto <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
---
 src/teavpn2/net/linux/iface.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/teavpn2/net/linux/iface.c b/src/teavpn2/net/linux/iface.c
index 010e195..a77c1c8 100644
--- a/src/teavpn2/net/linux/iface.c
+++ b/src/teavpn2/net/linux/iface.c
@@ -107,11 +107,14 @@ __cold static noinline char *escapeshellarg(char *alloc, const char *str,
 	size_t x;
 	char *cmd;
 
-	if (alloc == NULL)
+	if (alloc == NULL) {
 		/* Worst case */
 		cmd = (char *)malloc((sizeof(char) * l * 4) + 1);
-	else
+		if (!cmd)
+			return NULL;
+	} else {
 		cmd = alloc;
+	}
 
 #ifdef WIN32
 	cmd[y++] = '"';
-- 
Alviro Iskandar Setiawan


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH teavpn2 3/3] arch/linux: syscall: Fix retval checking in libc syscall
  2022-05-27  0:02 [PATCH teavpn2 0/3] teavpn2 fixes Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 1/3] allocator: Fix `@errno` value when overflow Alviro Iskandar Setiawan
  2022-05-27  0:02 ` [PATCH teavpn2 2/3] net: iface: Fix a potential NULL pointer dereference Alviro Iskandar Setiawan
@ 2022-05-27  0:02 ` Alviro Iskandar Setiawan
  2022-05-27  0:14 ` [PATCH teavpn2 0/3] teavpn2 fixes Ammar Faizi
  3 siblings, 0 replies; 5+ messages in thread
From: Alviro Iskandar Setiawan @ 2022-05-27  0:02 UTC (permalink / raw)
  To: Ammar Faizi
  Cc: Alviro Iskandar Setiawan, GNU/Weeb Mailing List,
	Tea Inside Mailing List, Ammar Faizi, Louvian Lyndal,
	Michael Arminto

The libc syscall wrappers mostly return -1 when they fail, then they
set the error code to the @errno variable. The current code seems to
be doing something wrong. We assume it errors when the return value
is negative. However, not all negative values are meant to be an
error indicator. On Linux, the only reserved error code is within
range [-4095, -1]. That means we still have a potential to get a
negative return value that is not an error.

I understand that most of them work fine here because of the nature
of the syscall itself that won't return a negative value upen
succcessful. But the above assumption about the negative value is
not correct.

Replace the error checking from (ret < 0) to (ret == -1) to reflect
the above fact.

Fixes: ca111cce3c05aed1d3a078c095c0111f3c48484f ("arch: Add generic arch syscalls from libc")
Cc: Ammar Faizi <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Cc: Michael Arminto <[email protected]>
Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
---
 src/teavpn2/arch/generic/linux.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/teavpn2/arch/generic/linux.h b/src/teavpn2/arch/generic/linux.h
index 42e7201..0dc12e8 100644
--- a/src/teavpn2/arch/generic/linux.h
+++ b/src/teavpn2/arch/generic/linux.h
@@ -20,21 +20,21 @@ static inline int __sys_epoll_wait(int epfd, struct epoll_event *events,
 {
 	int ret;
 	ret = epoll_wait(epfd, events, maxevents, timeout);
-	return unlikely(ret < 0) ? -errno : ret;
+	return unlikely(ret == -1) ? -errno : ret;
 }
 
 static inline ssize_t __sys_read(int fd, void *buf, size_t len)
 {
 	ssize_t ret;
 	ret = read(fd, buf, len);
-	return unlikely(ret < 0) ? (ssize_t) -errno : ret;
+	return unlikely(ret == -1) ? (ssize_t) -errno : ret;
 }
 
 static inline ssize_t __sys_write(int fd, const void *buf, size_t len)
 {
 	ssize_t ret;
 	ret = write(fd, buf, len);
-	return unlikely(ret < 0) ? (ssize_t) -errno : ret;
+	return unlikely(ret == -1) ? (ssize_t) -errno : ret;
 }
 
 static inline ssize_t __sys_recvfrom(int sockfd, void *buf, size_t len,
@@ -43,7 +43,7 @@ static inline ssize_t __sys_recvfrom(int sockfd, void *buf, size_t len,
 {
 	ssize_t ret;
 	ret = recvfrom(sockfd, buf, len, flags, src_addr, addrlen);
-	return unlikely(ret < 0) ? (ssize_t) -errno : ret;
+	return unlikely(ret == -1) ? (ssize_t) -errno : ret;
 }
 
 static inline ssize_t __sys_sendto(int sockfd, const void *buf, size_t len,
@@ -52,14 +52,14 @@ static inline ssize_t __sys_sendto(int sockfd, const void *buf, size_t len,
 {
 	ssize_t ret;
 	ret = sendto(sockfd, buf, len, flags, dest_addr, addrlen);
-	return unlikely(ret < 0) ? (ssize_t) -errno : ret;
+	return unlikely(ret == -1) ? (ssize_t) -errno : ret;
 }
 
 static inline int __sys_close(int fd)
 {
 	int ret;
 	ret = close(fd);
-	return unlikely(ret < 0) ? -errno : ret;
+	return unlikely(ret == -1) ? -errno : ret;
 }
 
 #endif /* #ifndef TEAVPN2__ARCH__GENERIC__LINUX_H */
-- 
Alviro Iskandar Setiawan


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH teavpn2 0/3] teavpn2 fixes
  2022-05-27  0:02 [PATCH teavpn2 0/3] teavpn2 fixes Alviro Iskandar Setiawan
                   ` (2 preceding siblings ...)
  2022-05-27  0:02 ` [PATCH teavpn2 3/3] arch/linux: syscall: Fix retval checking in libc syscall Alviro Iskandar Setiawan
@ 2022-05-27  0:14 ` Ammar Faizi
  3 siblings, 0 replies; 5+ messages in thread
From: Ammar Faizi @ 2022-05-27  0:14 UTC (permalink / raw)
  To: Alviro Iskandar Setiawan
  Cc: Ammar Faizi, Ammar Faizi, Tea Inside Mailing List,
	Michael Arminto, Louvian Lyndal, GNU/Weeb Mailing List

On Fri, 27 May 2022 00:02:24 +0000, Alviro Iskandar Setiawan wrote:
> I have a few TeaVPN2 fixes, please review. 3 patches below:
> 
> ### Patch 1
> The calloc() function from libc sets the @errno variable to ENOMEM
> when overflow, not to EOVERFLOW. Change it to ENOMEM to follow libc
> error code.
> 
> [...]

Applied, thanks!

[1/3] allocator: Fix `@errno` value when overflow
      commit: 3541908d4fe36522b6fbffec6d8455a824c89237
[2/3] net: iface: Fix a potential NULL pointer dereference
      commit: 7c9b07476d86fb27df835d977c07319954661a10
[3/3] arch/linux: syscall: Fix retval checking in libc syscall
      commit: c157b7221901d0b2f2ede5ab0f01471ecac91fed

Best regards,
-- 
Ammar Faizi


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-27  0:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27  0:02 [PATCH teavpn2 0/3] teavpn2 fixes Alviro Iskandar Setiawan
2022-05-27  0:02 ` [PATCH teavpn2 1/3] allocator: Fix `@errno` value when overflow Alviro Iskandar Setiawan
2022-05-27  0:02 ` [PATCH teavpn2 2/3] net: iface: Fix a potential NULL pointer dereference Alviro Iskandar Setiawan
2022-05-27  0:02 ` [PATCH teavpn2 3/3] arch/linux: syscall: Fix retval checking in libc syscall Alviro Iskandar Setiawan
2022-05-27  0:14 ` [PATCH teavpn2 0/3] teavpn2 fixes Ammar Faizi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox