[PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[Pablo Neira]

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

This patch introduces a new function called netlink_check_skb that does 
the sanity checkings for received messages.

--
Pablo

[-- Attachment #2: 01process_skb.patch --]
[-- Type: text/x-patch, Size: 2020 bytes --]

===== net/netlink/af_netlink.c 1.69 vs edited =====
--- 1.69/net/netlink/af_netlink.c	2005-01-21 21:25:32 +01:00
+++ edited/net/netlink/af_netlink.c	2005-02-10 00:37:57 +01:00
@@ -1201,6 +1201,42 @@
 	netlink_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).pid, MSG_DONTWAIT);
 }
 
+/* 
+ * Process one packet of messages.
+ * Malformed skbs with wrong lengths of messages are discarded silently.
+ */
+int netlink_process_skb(struct sk_buff *skb, 
+			int (*process_msg)(struct sk_buff *skb,
+					   struct nlmsghdr *nlh,
+					   int *err))
+{
+	int err;
+	struct nlmsghdr * nlh;
+
+	while (skb->len >= NLMSG_SPACE(0)) {
+		u32 rlen;
+
+		nlh = (struct nlmsghdr *)skb->data;
+		if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len)
+			return 0;
+		rlen = NLMSG_ALIGN(nlh->nlmsg_len);
+		if (rlen > skb->len)
+			rlen = skb->len;
+		if (process_msg(skb, nlh, &err)) {
+			/* Not error, but we must interrupt processing here:
+			 *   Note, that in this case we do not pull message
+			 *   from skb, it will be processed later.
+			 */
+			if (err == 0)
+				return -1;
+			netlink_ack(skb, nlh, err);
+		} else if (nlh->nlmsg_flags&NLM_F_ACK)
+			netlink_ack(skb, nlh, 0);
+		skb_pull(skb, rlen);
+	}
+
+	return 0;
+}
 
 #ifdef CONFIG_PROC_FS
 struct nl_seq_iter {
@@ -1456,6 +1492,7 @@
 
 MODULE_ALIAS_NETPROTO(PF_NETLINK);
 
+EXPORT_SYMBOL(netlink_process_skb);
 EXPORT_SYMBOL(netlink_ack);
 EXPORT_SYMBOL(netlink_broadcast);
 EXPORT_SYMBOL(netlink_dump_start);
--- linux-2.5/include/linux/netlink.h.orig	2005-02-10 00:48:55.000000000 +0100
+++ linux-2.5/include/linux/netlink.h	2005-02-10 00:49:40.000000000 +0100
@@ -119,6 +119,9 @@
 #define NETLINK_CREDS(skb)	(&NETLINK_CB((skb)).creds)
 
 
+extern int 
+netlink_process_skb(struct sk_buff *skb, int (*process_msg)(struct sk_buff *skb,
+					 struct nlmsghdr *nlh, int *err));
 extern struct sock *
 netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len));
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);

Re: [PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[Thomas Graf]

* Pablo Neira <420BF8CB.6080005@eurodev.net> 2005-02-11 01:14
> This patch introduces a new function called netlink_check_skb that does 
> the sanity checkings for received messages.

The patch description doesn't really match the patch itself.


> ===== net/netlink/af_netlink.c 1.69 vs edited =====
> --- 1.69/net/netlink/af_netlink.c	2005-01-21 21:25:32 +01:00
> +++ edited/net/netlink/af_netlink.c	2005-02-10 00:37:57 +01:00
> @@ -1201,6 +1201,42 @@
>  	netlink_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).pid, MSG_DONTWAIT);
>  }
>  
> +/* 
> + * Process one packet of messages.
> + * Malformed skbs with wrong lengths of messages are discarded silently.
> + */
> +int netlink_process_skb(struct sk_buff *skb, 
> +			int (*process_msg)(struct sk_buff *skb,
> +					   struct nlmsghdr *nlh,
> +					   int *err))
> +{
> +	int err;
> +	struct nlmsghdr * nlh;
> +
> +	while (skb->len >= NLMSG_SPACE(0)) {

While you're at it, change that to NLMSG_LENGTH(0) or even to
NLMSG_ALIGN(sizeof(*nlh)) to make it more readable. NLMSG_SPACE()
represents the total size of a netlink message in the byte stream
including the padding to payload in order to enforce proper
alignement for successive netlink message header.

Re: [PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[Pablo Neira]

Thomas Graf wrote:

>>+/* 
>>+ * Process one packet of messages.
>>+ * Malformed skbs with wrong lengths of messages are discarded silently.
>>+ */
>>+int netlink_process_skb(struct sk_buff *skb, 
>>+			int (*process_msg)(struct sk_buff *skb,
>>+					   struct nlmsghdr *nlh,
>>+					   int *err))
>>+{
>>+	int err;
>>+	struct nlmsghdr * nlh;
>>+
>>+	while (skb->len >= NLMSG_SPACE(0)) {
>>    
>>
>
>While you're at it, change that to NLMSG_LENGTH(0) or even to
>NLMSG_ALIGN(sizeof(*nlh)) to make it more readable. NLMSG_SPACE()
>represents the total size of a netlink message in the byte stream
>including the padding to payload in order to enforce proper
>alignement for successive netlink message header.
>  
>

They are all the same thing. They all return 16 bytes which is the size 
of a netlink header. If you and someone else think that those are more 
readable, I'm ok with it, whatever. I just stole that piece of code as 
is from current checkings done in xfrm and rtnetlink.

--
Pablo

Re: [PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[Thomas Graf]

> They are all the same thing. They all return 16 bytes which is the size 
> of a netlink header. If you and someone else think that those are more 
> readable, I'm ok with it, whatever. I just stole that piece of code as 
> is from current checkings done in xfrm and rtnetlink.

That's absolutely true, my point is that while reading that line
of code it reads like while the remaining length in the socket buffer
is still greather or equal than the total message size until the next
message with a payload of 0, then do something. It looks confusing at
a first glance because you're not event interested in the payload just
now. A choice of NLMSG_ALIGN(sizeof(*nlh)) or NLMSG_LENGTH(0) clearly
points out that you're just checking for enough room regarding the
netlink header in order to access it safely and do the real sanity
check against the complete message length. Engross that thought a bit
further, NLMSG_SPACE could change at some point assuming that it is
only used in contexts where the total message size is in question.

If you think that's too much of nitpicking, then forget about it. It's
technicaly absolutely correct at the moment.

Re: [PATCH 2/4] [NETLINK] introduce netlink_check_skb function

[Pablo Neira]

Hi Thomas,

Thomas Graf wrote:

>>They are all the same thing. They all return 16 bytes which is the size 
>>of a netlink header. If you and someone else think that those are more 
>>readable, I'm ok with it, whatever. I just stole that piece of code as 
>>is from current checkings done in xfrm and rtnetlink.
>>    
>>
>
>That's absolutely true, my point is that while reading that line
>of code it reads like while the remaining length in the socket buffer
>is still greather or equal than the total message size until the next
>message with a payload of 0, then do something. It looks confusing at
>a first glance because you're not event interested in the payload just
>now. A choice of NLMSG_ALIGN(sizeof(*nlh)) or NLMSG_LENGTH(0) clearly
>points out that you're just checking for enough room regarding the
>netlink header in order to access it safely and do the real sanity
>check against the complete message length. Engross that thought a bit
>further, NLMSG_SPACE could change at some point assuming that it is
>only used in contexts where the total message size is in question.
>
>If you think that's too much of nitpicking, then forget about it. It's
>technicaly absolutely correct at the moment.
>  
>

Looks fine you convince me. If this is finally pushed forward, let's 
replace that line. Thanks.

--
Pablo