[PATCH] Ensure check of nlmsg length is performed before actual access

[PATCH] Ensure check of nlmsg length is performed before actual access

[maxdev]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

During a brief code review we noticed that the length field expected 
inside the payload of the message is accessed before it is ensured that 
the payload is large enough to actually hold this field.

The people mentioned in the commit message helped in the overall code 
review.

Kind regards,
Max

[-- Attachment #2: 0001-Ensure-check-of-nlmsg-length-is-performed-before-act.patch --]
[-- Type: text/x-patch, Size: 952 bytes --]

From 89216bacbc44d6719668132626ffd66862be6dfc Mon Sep 17 00:00:00 2001
From: Max Kunzelmann <maxdev@posteo.de>
Date: Wed, 23 Mar 2022 20:42:58 +0100
Subject: [PATCH] Ensure check of nlmsg length is performed before actual
 access

Reviewed-by: Benny Baumann <BenBE@geshi.org>
Reviewed-by: Robert Geislinger <github@crpykng.de>
---
 lib/libnetlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 9af06232..0fe78943 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -732,13 +732,13 @@ int rtnl_dump_request_n(struct rtnl_handle *rth, struct nlmsghdr *n)
 static int rtnl_dump_done(struct nlmsghdr *h,
 			  const struct rtnl_dump_filter_arg *a)
 {
-	int len = *(int *)NLMSG_DATA(h);
-
 	if (h->nlmsg_len < NLMSG_LENGTH(sizeof(int))) {
 		fprintf(stderr, "DONE truncated\n");
 		return -1;
 	}
 
+	int len = *(int *)NLMSG_DATA(h);
+
 	if (len < 0) {
 		errno = -len;
 
-- 
2.38.1

Re: [PATCH] Ensure check of nlmsg length is performed before actual access

[Jacob Keller]

On 11/30/2022 2:09 PM, maxdev@posteo.de wrote:
> During a brief code review we noticed that the length field expected 
> inside the payload of the message is accessed before it is ensured that 
> the payload is large enough to actually hold this field.
> 
> The people mentioned in the commit message helped in the overall code 
> review.
> 
> Kind regards,
> Max

Hi,

Typically patches would be sent directly as plain text in the email 
content, and not as an attachment.

As this is a fix, you would also typically determine what commit this 
fixes, and add a "Fixes:" trailer to indicate this. You should also have 
the subject include either "net" or "net-next" along with PATCH inside 
the [].

As for the patch contents itself, Linux still follows C89 rules for 
declarations, and you should leave "int len" at the top of the scope, 
but assign it after the validation check as you do.

Thanks,
Jake