Skip to contentSkip to conversation

introduce netlink_check_skb function

PATCH In review Pablo Neira 2 participants 5 messages Feb 11, 2005 – Feb 12, 2005

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 2/4 introduce netlink_check_skb function

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

--
Pablo

===== net/netlink/af_netlink.c 1.69 vs edited =====

net/netlink/af_netlink.c +37 -0
@@ -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);
include/linux/netlink.h renamed from include/linux/netlink.h.orig +3 -0
@@ -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);
#1 lore raw

* Pablo Neira <[email protected]> 2005-02-11 01:14

Quoted 2 lines

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.

Quoted 10 lines

===== net/netlink/af_netlink.c 1.69 vs edited =====

net/netlink/af_netlink.c +3 -0
@@ -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.

net/netlink/af_netlink.c line 1216

Quoted 10 lines
net/netlink/af_netlink.c +10 -0
+ */
+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.

author
#2 lore raw
Quoted 13 lines · Thomas Graf
Quoted 13 lines
net/netlink/af_netlink.c +13 -0
+/* 
+ * 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)) {
Quoted 10 lines
Quoted 2 lines

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

#3 lore raw
Quoted 4 lines

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.

author
#4 lore raw

Hi Thomas,

Quoted 16 lines · Thomas Graf
Quoted 3 lines

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.

Quoted 4 lines

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