[PATCH] audit: fix size of netlink messages

[PATCH] audit: fix size of netlink messages

[Nicolas Dichtel]

Netlink messages must be aligned on NLMSG_ALIGNTO (4 bytes), thus we need to
update the skb length before sending it to userspace.

This patch adds the needed padding to be compliant with this requirement.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 kernel/audit.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 21c7fa6..31d213a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1669,6 +1669,17 @@ void audit_log_end(struct audit_buffer *ab)
 		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
 		nlh->nlmsg_len = ab->skb->len - NLMSG_HDRLEN;
 
+		if (NLMSG_ALIGN(ab->skb->len) != ab->skb->len) {
+			unsigned int pad = NLMSG_ALIGN(ab->skb->len) -
+					   ab->skb->len;
+
+			if (skb_tailroom(ab->skb) >= pad)
+				skb_put(ab->skb, pad);
+			else if (pskb_expand_head(ab->skb, 0, pad,
+						  GFP_KERNEL) < 0)
+				audit_log_lost("out of memory in audit_log_end");
+		}
+
 		if (audit_pid) {
 			skb_queue_tail(&audit_skb_queue, ab->skb);
 			wake_up_interruptible(&kauditd_wait);
-- 
1.8.2.1

Re: [PATCH] audit: fix size of netlink messages

[Eric Paris]

On Fri, 2013-06-07 at 17:25 +0200, Nicolas Dichtel wrote:

NAK.

I tried this once before and as I recal userspace actually expected the
stoopidity of being unaligned and broke   :-(

> Netlink messages must be aligned on NLMSG_ALIGNTO (4 bytes), thus we need to
> update the skb length before sending it to userspace.
> 
> This patch adds the needed padding to be compliant with this requirement.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  kernel/audit.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 21c7fa6..31d213a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1669,6 +1669,17 @@ void audit_log_end(struct audit_buffer *ab)
>  		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
>  		nlh->nlmsg_len = ab->skb->len - NLMSG_HDRLEN;
>  
> +		if (NLMSG_ALIGN(ab->skb->len) != ab->skb->len) {
> +			unsigned int pad = NLMSG_ALIGN(ab->skb->len) -
> +					   ab->skb->len;
> +
> +			if (skb_tailroom(ab->skb) >= pad)
> +				skb_put(ab->skb, pad);
> +			else if (pskb_expand_head(ab->skb, 0, pad,
> +						  GFP_KERNEL) < 0)
> +				audit_log_lost("out of memory in audit_log_end");
> +		}
> +
>  		if (audit_pid) {
>  			skb_queue_tail(&audit_skb_queue, ab->skb);
>  			wake_up_interruptible(&kauditd_wait);

Re: [PATCH] audit: fix size of netlink messages

[Nicolas Dichtel]

Put Thomas in CC.

Le 07/06/2013 17:43, Eric Paris a écrit :
> On Fri, 2013-06-07 at 17:25 +0200, Nicolas Dichtel wrote:
>
> NAK.
>
> I tried this once before and as I recal userspace actually expected the
> stoopidity of being unaligned and broke   :-(
On which userspace tools do you think?

For example, in the libnl, the function which tries to get the next netlink 
message expects this alignment:

struct nlmsghdr *nlmsg_next(struct nlmsghdr *nlh, int *remaining)
{
         int totlen = NLMSG_ALIGN(nlh->nlmsg_len);

         *remaining -= totlen;

         return (struct nlmsghdr *) ((unsigned char *) nlh + totlen);
}

http://www.infradead.org/~tgr/libnl/doc/core.html#_message_parsing_amp_construction

>
>> Netlink messages must be aligned on NLMSG_ALIGNTO (4 bytes), thus we need to
>> update the skb length before sending it to userspace.
>>
>> This patch adds the needed padding to be compliant with this requirement.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   kernel/audit.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 21c7fa6..31d213a 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1669,6 +1669,17 @@ void audit_log_end(struct audit_buffer *ab)
>>   		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
>>   		nlh->nlmsg_len = ab->skb->len - NLMSG_HDRLEN;
>>
>> +		if (NLMSG_ALIGN(ab->skb->len) != ab->skb->len) {
Here, I think it's better to use nlmsg_padlen().

>> +			unsigned int pad = NLMSG_ALIGN(ab->skb->len) -
>> +					   ab->skb->len;
>> +
>> +			if (skb_tailroom(ab->skb) >= pad)
>> +				skb_put(ab->skb, pad);
>> +			else if (pskb_expand_head(ab->skb, 0, pad,
>> +						  GFP_KERNEL) < 0)
>> +				audit_log_lost("out of memory in audit_log_end");
>> +		}
>> +
>>   		if (audit_pid) {
>>   			skb_queue_tail(&audit_skb_queue, ab->skb);
>>   			wake_up_interruptible(&kauditd_wait);
>
>

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: fix size of netlink messages

[Nicolas Dichtel]

Le 09/06/2013 15:29, Thomas Graf a écrit :
> On 06/07/13 at 06:24pm, Nicolas Dichtel wrote:
>> Put Thomas in CC.
>>
>> Le 07/06/2013 17:43, Eric Paris a écrit :
>>> On Fri, 2013-06-07 at 17:25 +0200, Nicolas Dichtel wrote:
>>>
>>> NAK.
>>>
>>> I tried this once before and as I recal userspace actually expected the
>>> stoopidity of being unaligned and broke   :-(
>> On which userspace tools do you think?
>>
>> For example, in the libnl, the function which tries to get the next
>> netlink message expects this alignment:
>
> You should only align the end of the Netlink message if you
> append another Netlink message after it. There is no point
> in aligning it if no message follows in the same buffer.
>
> A receiver should never enforce alignment for the last message.
> Some do and this is causing a lot of pain. An example is the
> Open vSwitch implementation which stricly enforces alignment of
> nlmsg_len which makes attempts to implement zerocopy for the
> upcall more difficult than it should be.
>
Ok, thank you for the clarification.