7:19 a.m.
Hi,
I was looking through the source to the .88 kernel and ran across this:
static int audit_receive_skb(struct sk_buff *skb)
{
int err;
struct nlmsghdr *nlh;
u32 rlen;
while (skb->len >= NLMSG_SPACE(0)) {
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 ((err = audit_receive_msg(skb, nlh))) {
netlink_ack(skb, nlh, err);
} else if (nlh->nlmsg_flags & NLM_F_ACK)
netlink_ack(skb, nlh, 0);
skb_pull(skb, rlen);
}
return 0;
}
It only returns 0. Is this a mistake or should this be made void? The reason I
ask is that the return code is used like this:
if (audit_receive_skb(skb) && skb->len)
skb_queue_head(&sk->sk_receive_queue, skb);
else
kfree_skb(skb);
The way the code is, we will never put the skb back on the queue head. Should
this be refactored or do we have a problem in the .88 kernel?
-Steve
11:36 a.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
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 ((err = audit_receive_msg(skb, nlh))) {
netlink_ack(skb, nlh, err);
} else if (nlh->nlmsg_flags & NLM_F_ACK)
netlink_ack(skb, nlh, 0);
skb_pull(skb, rlen);
}
return 0;
}
It only returns 0. Is this a mistake or should this be made void? The reason
I ask is that the return code is used like this:
if (audit_receive_skb(skb) && skb->len)
skb_queue_head(&sk->sk_receive_queue, skb);
else
kfree_skb(skb);
The way the code is, we will never put the skb back on the queue head. Should
this be refactored or do we have a problem in the .88 kernel?
That should just mean we're dropping bad packets. It's void upstream, btw.
thanks,
-chris
12:05 p.m.
On Monday 12 September 2005 12:36, Chris Wright wrote:
That should just mean we're dropping bad packets.
Where would we detect the packet is bad?
It's void upstream, btw.
If something can go wrong, we should detect the problem and return 1. If there
is nothing that can go wrong, we should make it void. I guess that's what I'm
getting at.
-Steve
12:28 p.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
On Monday 12 September 2005 12:36, Chris Wright wrote:
> That should just mean we're dropping bad packets.
Where would we detect the packet is bad?
In aduit_receive_skb (just basic sanity checks on skb length and netlink
msg header info).
> It's void upstream, btw.
If something can go wrong, we should detect the problem and return 1. If there
is nothing that can go wrong, we should make it void. I guess that's what I'm
getting at.
Going wrong == packet we can't handle. So we just drop it. I agree,
and it's already done upstream, but I don't think it's worth spinning
a new kernel for.
12:33 p.m.
On Monday 12 September 2005 13:28, Chris Wright wrote:
In aduit_receive_skb (just basic sanity checks on skb length and
netlink
msg header info).
OK, I see.
> > It's void upstream, btw.
>
> If something can go wrong, we should detect the problem and return 1. If
> there is nothing that can go wrong, we should make it void. I guess
> that's what I'm getting at.
Going wrong == packet we can't handle. So we just drop it.
Does this send a NACK back to auditctl?
I agree, and it's already done upstream, but I don't think
it's worth
spinning a new kernel for.
I think we are at the point where we are starting the new development. Maybe
this doesn't go into a .89 kernel, but a .1 kernel for the new development
cycle.
Thanks,
-Steve
12:58 p.m.
* Steve Grubb (sgrubb(a)redhat.com) wrote:
On Monday 12 September 2005 13:28, Chris Wright wrote:
> Going wrong == packet we can't handle. So we just drop it.
Does this send a NACK back to auditctl?
No, it doesn't, but this is the case where the netlink message header
in the skb is so meaningless that we can glean nothing useful from it
(including say, serial number that we'd NACK).
> I agree, and it's already done upstream, but I don't
think it's worth
> spinning a new kernel for.
I think we are at the point where we are starting the new development. Maybe
this doesn't go into a .89 kernel, but a .1 kernel for the new development
cycle.
Yeah, it should be in sync with upstream for the new development.
thanks,
-chris
7040
days inactive
7041
days old
linux-audit@lists.linux-audit.osci.io
5 comments
2 participants
participants (2)
-
Chris Wright -
Steve Grubb