Re: Sending short raw packets using sendmsg() broke
On Fri, Mar 4, 2016 at 11:33 AM, Willem de Bruijnwrote: > On Fri, Mar 4, 2016 at 10:54 AM, Alan Cox wrote: >>> > A quick search for ethhdr in drivers/net/ethernet shows, for >>> > instance, >>> > bnx2x_select_queue casting skb->data to an ethernet header. Reading >>> > nonsense in that particular function is quite safe and given the >>> > skbuff layout (skb_shared_info) code will never read beyond an >>> > allocated region. But that was just the first occurrence I found. >>> > efx_tso_check_protocol is another example. >> >> So would always allocating that much space be a good mitigation in >> general > > Agreed. The existing packet allocation path does that by relying on > LL_RESERVED_SPACE on allocation, not only packet length. I won't > modify that. But I should perhaps zero up to hard_header_len on > variable length headers. > >> and perhaps then making the logic check validate() IFF >> CAP_SYS_RAWIO is not set. >> >> A user with CAP_SYS_RAWIO already has the power to control the device >> by banging registers so the check is not a security loss. > > One concern is namespaces. I'll use capable(CAP_SYS_RAWIO), not ns_capable. > > Need to add an ax25_validate implemention and run some tests before I > send out the patch. Sent a v1. I will try to test the ax25 specific code. Heikki, if you can, please test it, too. The change to tpacket_fill_skb will cause a merge conflict with net-next because of my tpacket gso changes in http://patchwork.ozlabs.org/patch/578623/ > > Thanks, > > Willem
Re: Sending short raw packets using sendmsg() broke
On Fri, Mar 4, 2016 at 10:54 AM, Alan Coxwrote: >> > A quick search for ethhdr in drivers/net/ethernet shows, for >> > instance, >> > bnx2x_select_queue casting skb->data to an ethernet header. Reading >> > nonsense in that particular function is quite safe and given the >> > skbuff layout (skb_shared_info) code will never read beyond an >> > allocated region. But that was just the first occurrence I found. >> > efx_tso_check_protocol is another example. > > So would always allocating that much space be a good mitigation in > general Agreed. The existing packet allocation path does that by relying on LL_RESERVED_SPACE on allocation, not only packet length. I won't modify that. But I should perhaps zero up to hard_header_len on variable length headers. > and perhaps then making the logic check validate() IFF > CAP_SYS_RAWIO is not set. > > A user with CAP_SYS_RAWIO already has the power to control the device > by banging registers so the check is not a security loss. One concern is namespaces. I'll use capable(CAP_SYS_RAWIO), not ns_capable. Need to add an ax25_validate implemention and run some tests before I send out the patch. Thanks, Willem
Re: Sending short raw packets using sendmsg() broke
> > A quick search for ethhdr in drivers/net/ethernet shows, for > > instance, > > bnx2x_select_queue casting skb->data to an ethernet header. Reading > > nonsense in that particular function is quite safe and given the > > skbuff layout (skb_shared_info) code will never read beyond an > > allocated region. But that was just the first occurrence I found. > > efx_tso_check_protocol is another example. So would always allocating that much space be a good mitigation in general, and perhaps then making the logic check validate() IFF CAP_SYS_RAWIO is not set. A user with CAP_SYS_RAWIO already has the power to control the device by banging registers so the check is not a security loss. Alan
Re: Sending short raw packets using sendmsg() broke
On Thu, Mar 3, 2016 at 11:40 AM, Willem de Bruijnwrote: > On Tue, Mar 1, 2016 at 7:00 PM, Alan Cox wrote: >>> More thorough validation of the header contents is not necessarily >>> hard. The following validates the address, including optional >>> repeaters. >>> >>> static bool ax25_validate_hard_header(const char *ll_header, >>>unsigned short len) >>> { >>> ax25_digi digi; >>> >>> return !ax25_addr_parse(ll_header, len, NULL, NULL, , >>> NULL, NULL); >>> } >> >> This also breaks because there is a KISS header byte on an AX.25 >> transmission and it is valid to send a KISS control frame via >> SOCK_PACKET but it cannot be generated by other protocols. >> >> Basically everything hitting an AX.25 port is either a zero byte >> followed by an AX.25 frame, or a KISS frame the first of which is non >> zero and which is used to set parameters on the radio side. >> >> The AX.25 device level drivers are simply written to be robust if >> thrown partial frames. > > That is preferable, but unfortunately does not seem to be true in general. > > A quick search for ethhdr in drivers/net/ethernet shows, for instance, > bnx2x_select_queue casting skb->data to an ethernet header. Reading > nonsense in that particular function is quite safe and given the > skbuff layout (skb_shared_info) code will never read beyond an > allocated region. But that was just the first occurrence I found. > efx_tso_check_protocol is another example. > > The stack itself also has a few unconditional uses of > dev->hard_header_len as lower bound on packet length. > dump_ipv4_mac_header in net/ipv4/netfilter/nf_log_ipv4.c iterates > over bytes and logs them to the system log. nla_put(inst->skb, > FULA_HWHEADER, skb->dev->hard_header_len, hwhdrp) in > net/netfilter/nfnetlink_log passes bytes up to userspace. With > ebtables or tc + act_ipt, it might be possible to construct a path > from a packet socket through one of these. I'm not sure. Regardless of > the immediate fix, these should probably be made more robust against > short packets. > >> The other thing that concerns me about this added logic in general is >> that you are also breaking test tools that want to deliberately send >> corrupt frames to certain classes of interface. I'm not sure how big an >> issue that is given we always for example padded ethernet frames >> properly, but the more validation we do for a privileged interface the >> more we prevent applications for testing network behaviour from being >> able to run on Linux. > > Good point. Given how a minimum header length check already causes so > much problems, I hesitate to add more validation logic > unconditionally. > >> Possibly there should be a CAP_SYS_RAWIO test but >> making it impossible is a bad step. > > Okay. To avoid overloading this capability, perhaps a per-device > sysctl analogous to net.ipv4.conf.$DEV.accept_local? That per device space does not exist for net.core, so it would have to be a global option (net.core.validate_ll_hdr) > > I'll start with the patch to replaces ll_header_truncate with a > validate() + a separate minimal ax25 implementation. > >> >> Alan >> >> >> >> >> >> >>
Re: Sending short raw packets using sendmsg() broke
On Tue, Mar 1, 2016 at 7:00 PM, Alan Coxwrote: >> More thorough validation of the header contents is not necessarily >> hard. The following validates the address, including optional >> repeaters. >> >> static bool ax25_validate_hard_header(const char *ll_header, >>unsigned short len) >> { >> ax25_digi digi; >> >> return !ax25_addr_parse(ll_header, len, NULL, NULL, , >> NULL, NULL); >> } > > This also breaks because there is a KISS header byte on an AX.25 > transmission and it is valid to send a KISS control frame via > SOCK_PACKET but it cannot be generated by other protocols. > > Basically everything hitting an AX.25 port is either a zero byte > followed by an AX.25 frame, or a KISS frame the first of which is non > zero and which is used to set parameters on the radio side. > > The AX.25 device level drivers are simply written to be robust if > thrown partial frames. That is preferable, but unfortunately does not seem to be true in general. A quick search for ethhdr in drivers/net/ethernet shows, for instance, bnx2x_select_queue casting skb->data to an ethernet header. Reading nonsense in that particular function is quite safe and given the skbuff layout (skb_shared_info) code will never read beyond an allocated region. But that was just the first occurrence I found. efx_tso_check_protocol is another example. The stack itself also has a few unconditional uses of dev->hard_header_len as lower bound on packet length. dump_ipv4_mac_header in net/ipv4/netfilter/nf_log_ipv4.c iterates over bytes and logs them to the system log. nla_put(inst->skb, FULA_HWHEADER, skb->dev->hard_header_len, hwhdrp) in net/netfilter/nfnetlink_log passes bytes up to userspace. With ebtables or tc + act_ipt, it might be possible to construct a path from a packet socket through one of these. I'm not sure. Regardless of the immediate fix, these should probably be made more robust against short packets. > The other thing that concerns me about this added logic in general is > that you are also breaking test tools that want to deliberately send > corrupt frames to certain classes of interface. I'm not sure how big an > issue that is given we always for example padded ethernet frames > properly, but the more validation we do for a privileged interface the > more we prevent applications for testing network behaviour from being > able to run on Linux. Good point. Given how a minimum header length check already causes so much problems, I hesitate to add more validation logic unconditionally. > Possibly there should be a CAP_SYS_RAWIO test but > making it impossible is a bad step. Okay. To avoid overloading this capability, perhaps a per-device sysctl analogous to net.ipv4.conf.$DEV.accept_local? I'll start with the patch to replaces ll_header_truncate with a validate() + a separate minimal ax25 implementation. > > Alan > > > > > > >
Re: Sending short raw packets using sendmsg() broke
> More thorough validation of the header contents is not necessarily > hard. The following validates the address, including optional > repeaters. > > static bool ax25_validate_hard_header(const char *ll_header, >unsigned short len) > { > ax25_digi digi; > > return !ax25_addr_parse(ll_header, len, NULL, NULL, , > NULL, NULL); > } This also breaks because there is a KISS header byte on an AX.25 transmission and it is valid to send a KISS control frame via SOCK_PACKET but it cannot be generated by other protocols. Basically everything hitting an AX.25 port is either a zero byte followed by an AX.25 frame, or a KISS frame the first of which is non zero and which is used to set parameters on the radio side. The AX.25 device level drivers are simply written to be robust if thrown partial frames. The other thing that concerns me about this added logic in general is that you are also breaking test tools that want to deliberately send corrupt frames to certain classes of interface. I'm not sure how big an issue that is given we always for example padded ethernet frames properly, but the more validation we do for a privileged interface the more we prevent applications for testing network behaviour from being able to run on Linux. Possibly there should be a CAP_SYS_RAWIO test but making it impossible is a bad step. Alan
Re: Sending short raw packets using sendmsg() broke
On Fri, 2016-02-26 at 12:33 -0500, Willem de Bruijn wrote: > On Fri, Feb 26, 2016 at 9:44 AM, Alan Cox> wrote: > > On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote: > > > From: Heikki Hannikainen > > > Date: Thu, 25 Feb 2016 21:36:07 +0200 (EET) > > > > > > > Commit 9c7077622dd9174 added a check, ll_header_truncated(), > > > > which > > > > requires that a packet transmitted using sendmsg() with > > > > PF_PACKET, > > > > SOCK_RAW must be longer than dev->hard_header_len. > > > > > > Fixed by: > > > > > > commit 880621c2605b82eb5af91a2c94223df6f5a3fb64 > > > Author: Martin Blumenstingl > > > Date: Sun Nov 22 17:46:09 2015 +0100 > > > > > > packet: Allow packets with only a header (but no payload) > > > > The AX.25 case the header is variable length so this still doesn't > > fix > > the regression as far as I can see. > > Right. The simplest, if hacky, fix is to add something along the > lines of > > static unsigned short netdev_min_hard_header_len(struct net_device > *dev) > { > if (unlikely(dev->type ==ARPHDR_AX25)) > return AX25_KISS_HEADER_LEN; > else > return dev->hard_header_len; > } AX.25 is not unique in this. Also there are protocols where the minimum header length for a valid raw frame is not the same as the minimum sized header for encapsulation of an IP frame because the IP frame is encapsulated with an extra header block. > Depending on how the variable encoding scheme works, a basic min > > length check may still produce buggy headers that confuse the stack > or > driver. I need to read up on AX25. If so, then extending header_ops > with an optional validate() function is a more generic approach of > checking header sanity. A validate() method is doable for NetROM, AX.25 and friends. So something like if (likely(len >= dev->hard_header_len)) return good; if (proto->validate && proto->validate(skb)) return good; return bad; works for amateur radio at least, and I think could be extended ok for any other cases like tunnels. Alan
Re: Sending short raw packets using sendmsg() broke
On Fri, Feb 26, 2016 at 12:46 PM, David Millerwrote: > From: Willem de Bruijn > Date: Fri, 26 Feb 2016 12:33:13 -0500 > >> Right. The simplest, if hacky, fix is to add something along the lines of >> >> static unsigned short netdev_min_hard_header_len(struct net_device *dev) >> { >> if (unlikely(dev->type ==ARPHDR_AX25)) >> return AX25_KISS_HEADER_LEN; >> else >> return dev->hard_header_len; >> } >> >> Depending on how the variable encoding scheme works, a basic min >> length check may still produce buggy headers that confuse the stack or >> driver. I need to read up on AX25. If so, then extending header_ops >> with an optional validate() function is a more generic approach of >> checking header sanity. > > I suspect we will need some kind of header ops for this. To return the device type minimum length or to do full header validation? Looking at drivers/net/hamradio, I don't see any driver output paths interpreting the header fields, in which case the first is sufficient. A minimum U/S frame is AX25_KISS_HEADER_LEN + 2* AX25_ADDR_LEN + 3 (control + FCS) == AX25_KISS_HEADER_LEN + AX25_HEADER_LEN Heikki, you gave this number + 3. Where does that constant come from? More thorough validation of the header contents is not necessarily hard. The following validates the address, including optional repeaters. static bool ax25_validate_hard_header(const char *ll_header, unsigned short len) { ax25_digi digi; return !ax25_addr_parse(ll_header, len, NULL, NULL, , NULL, NULL); } The major drawback of full validation from the point of fixing the original bug that it requires the header already having been copied to the kernel. The ll_header_truncated check is currently performed before allocation + copy, based solely on len. So this might become a relatively complex patch that is not easy to backport to stable branches. I can send simple minimal length validation patch to net to solve the reported bug. Then optionally follow up with a header_ops->validate() extension in net-next, if there is value in that.
Re: Sending short raw packets using sendmsg() broke
From: Willem de BruijnDate: Fri, 26 Feb 2016 12:33:13 -0500 > Right. The simplest, if hacky, fix is to add something along the lines of > > static unsigned short netdev_min_hard_header_len(struct net_device *dev) > { > if (unlikely(dev->type ==ARPHDR_AX25)) > return AX25_KISS_HEADER_LEN; > else > return dev->hard_header_len; > } > > Depending on how the variable encoding scheme works, a basic min > length check may still produce buggy headers that confuse the stack or > driver. I need to read up on AX25. If so, then extending header_ops > with an optional validate() function is a more generic approach of > checking header sanity. I suspect we will need some kind of header ops for this.
Re: Sending short raw packets using sendmsg() broke
From: Alan CoxDate: Fri, 26 Feb 2016 14:44:34 + > On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote: >> From: Heikki Hannikainen >> Date: Thu, 25 Feb 2016 21:36:07 +0200 (EET) >> >> > Commit 9c7077622dd9174 added a check, ll_header_truncated(), which >> > requires that a packet transmitted using sendmsg() with PF_PACKET, >> > SOCK_RAW must be longer than dev->hard_header_len. >> >> Fixed by: >> >> commit 880621c2605b82eb5af91a2c94223df6f5a3fb64 >> Author: Martin Blumenstingl >> Date: Sun Nov 22 17:46:09 2015 +0100 >> >> packet: Allow packets with only a header (but no payload) > > The AX.25 case the header is variable length so this still doesn't fix > the regression as far as I can see. Then can you suggest a way to ensure that the user has given us a fully specified link header? Perhaps we can have a netdev_ops callback for this, that variable length header technologies can implement.
Re: Sending short raw packets using sendmsg() broke
On Fri, Feb 26, 2016 at 9:44 AM, Alan Coxwrote: > On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote: >> From: Heikki Hannikainen >> Date: Thu, 25 Feb 2016 21:36:07 +0200 (EET) >> >> > Commit 9c7077622dd9174 added a check, ll_header_truncated(), which >> > requires that a packet transmitted using sendmsg() with PF_PACKET, >> > SOCK_RAW must be longer than dev->hard_header_len. >> >> Fixed by: >> >> commit 880621c2605b82eb5af91a2c94223df6f5a3fb64 >> Author: Martin Blumenstingl >> Date: Sun Nov 22 17:46:09 2015 +0100 >> >> packet: Allow packets with only a header (but no payload) > > The AX.25 case the header is variable length so this still doesn't fix > the regression as far as I can see. Right. The simplest, if hacky, fix is to add something along the lines of static unsigned short netdev_min_hard_header_len(struct net_device *dev) { if (unlikely(dev->type ==ARPHDR_AX25)) return AX25_KISS_HEADER_LEN; else return dev->hard_header_len; } Depending on how the variable encoding scheme works, a basic min length check may still produce buggy headers that confuse the stack or driver. I need to read up on AX25. If so, then extending header_ops with an optional validate() function is a more generic approach of checking header sanity. Assuming that validate() is not needed, I can code up the above and send it for review if no one objects. A third option is to add an explicit min_hard_header_len to net_device and use that in ll_header_truncated.
Re: Sending short raw packets using sendmsg() broke
On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote: > From: Heikki Hannikainen> Date: Thu, 25 Feb 2016 21:36:07 +0200 (EET) > > > Commit 9c7077622dd9174 added a check, ll_header_truncated(), which > > requires that a packet transmitted using sendmsg() with PF_PACKET, > > SOCK_RAW must be longer than dev->hard_header_len. > > Fixed by: > > commit 880621c2605b82eb5af91a2c94223df6f5a3fb64 > Author: Martin Blumenstingl > Date: Sun Nov 22 17:46:09 2015 +0100 > > packet: Allow packets with only a header (but no payload) The AX.25 case the header is variable length so this still doesn't fix the regression as far as I can see. Alan
Re: Sending short raw packets using sendmsg() broke
> Commit 9c7077622dd9174 added a check, ll_header_truncated(), which requires > that a packet transmitted using sendmsg() with PF_PACKET, SOCK_RAW must be > longer than dev->hard_header_len. > > https://github.com/torvalds/linux/commit/9c7077622dd9174 > https://github.com/torvalds/linux/blob/master/net/packet/af_packet.c#L2329 As David already pointed out, this has been revised to allow greater than or equal. Note that the behavior was already present for tpacket_snd and this patch only rationalized the two paths. > > The bug that popped up is that an application (aprx) can no longer send > short AX.25 packets using sendmsg(). Packets shorter than 77 bytes fail this > check in ll_header_truncated(). With older kernels, no problem. AX.25 (and > some other protocols) have variable-length headers (somewhere between 21 and > 77 bytes in this case). > > hard_header_len is set in drivers/net/hamradio/mkiss.c to > AX25_KISS_HEADER_LEN + AX25_MAX_HEADER_LEN + 3 which works out to be If header length is variable and can be shorter than AX25_MAX_HEADER_LEN then the check will still trigger. > (1+17+7*8+3)=77. > > https://github.com/torvalds/linux/blob/master/drivers/net/hamradio/mkiss.c#L845 > > I guessed that we could probably set hard_header_len to be the minimum > length of the packet header (AX25_KISS_HEADER_LEN + AX25_HEADER_LEN + 3) to > make things work again, but I first asked Alan Cox for an opinion, and he > says hard_header_len is set correctly to be the worst-case maximum header > length, and that the ll_header_truncated commit should be reverted instead, > since it doesn't take variable-length headers into account. hard_header_length is used in cases where we have to reserve room or check against packets that exceed frame size, so it indeed should not be changed to be the minimum header length in a variable header length scenario. In most protocols the header length is fixed, so there is no separate field for minimal header length. If this is the only such check in the kernel (and I haven't found another after a cursory inspection), then perhaps an exception should be made here for this one protocol family. > s = socket(AF_AX25, SOCK_DGRAM, 0); > > I didn't yet figure out why that works, maybe the sendto() of an AX25 > datagram does not go through that hard_header_len check. That is a different protocol family. The above check is limited to sends over packets of family PF_PACKET.
Re: Sending short raw packets using sendmsg() broke
From: Heikki HannikainenDate: Thu, 25 Feb 2016 21:36:07 +0200 (EET) > Commit 9c7077622dd9174 added a check, ll_header_truncated(), which > requires that a packet transmitted using sendmsg() with PF_PACKET, > SOCK_RAW must be longer than dev->hard_header_len. Fixed by: commit 880621c2605b82eb5af91a2c94223df6f5a3fb64 Author: Martin Blumenstingl Date: Sun Nov 22 17:46:09 2015 +0100 packet: Allow packets with only a header (but no payload)
Sending short raw packets using sendmsg() broke
Hi, Commit 9c7077622dd9174 added a check, ll_header_truncated(), which requires that a packet transmitted using sendmsg() with PF_PACKET, SOCK_RAW must be longer than dev->hard_header_len. https://github.com/torvalds/linux/commit/9c7077622dd9174 https://github.com/torvalds/linux/blob/master/net/packet/af_packet.c#L2329 The bug that popped up is that an application (aprx) can no longer send short AX.25 packets using sendmsg(). Packets shorter than 77 bytes fail this check in ll_header_truncated(). With older kernels, no problem. AX.25 (and some other protocols) have variable-length headers (somewhere between 21 and 77 bytes in this case). hard_header_len is set in drivers/net/hamradio/mkiss.c to AX25_KISS_HEADER_LEN + AX25_MAX_HEADER_LEN + 3 which works out to be (1+17+7*8+3)=77. https://github.com/torvalds/linux/blob/master/drivers/net/hamradio/mkiss.c#L845 I guessed that we could probably set hard_header_len to be the minimum length of the packet header (AX25_KISS_HEADER_LEN + AX25_HEADER_LEN + 3) to make things work again, but I first asked Alan Cox for an opinion, and he says hard_header_len is set correctly to be the worst-case maximum header length, and that the ll_header_truncated commit should be reverted instead, since it doesn't take variable-length headers into account. - Hessu -- Forwarded message -- From: Heikki HannikainenTo: Aprx software Date: Thu, 25 Feb 2016 11:22:05 +0200 (EET) Subject: Re: packet size is too short Kernel Error with Aprx Hi, I spent a bit of time trying to understand what's happening. As described by others, if the packet being transmitted is short, the newer Linux kernels drop it, saying this in the kernel log (dmesg): [405809.774704] aprx: packet size is too short (59 <= 77) The check in the kernel is here: https://github.com/torvalds/linux/blob/master/net/packet/af_packet.c#L2329 The check requires that the packet is longer than dev->hard_header_len. hard_header_len is set in linux drivers/net/hamradio/mkiss.c to AX25_KISS_HEADER_LEN + AX25_MAX_HEADER_LEN + 3 which works out to be (1+17+7*8+3)=77: #define AX25_MAX_DIGIS 8 #define AX25_HEADER_LEN 17 #define AX25_ADDR_LEN 7 #define AX25_DIGI_HEADER_LEN(AX25_MAX_DIGIS * AX25_ADDR_LEN) #define AX25_MAX_HEADER_LEN (AX25_HEADER_LEN + AX25_DIGI_HEADER_LEN) https://github.com/torvalds/linux/blob/master/drivers/net/hamradio/mkiss.c#L845 aprx uses the sendmsg() system call to send raw, variable-length-header AX25 frames, and those may well be shorter than 77 bytes, if there are not many digipeaters in the path and the packet payload is short (not a bad idea on a 1200 bit/s channel). https://github.com/PhirePhly/aprx/blob/master/netax25.c#L758 It may be that hard_header_len should be set in mkiss.c to AX25_KISS_HEADER_LEN + AX25_HEADER_LEN + 3 instead, if I understood this right. From include/linux/netdevice.h: * @hard_header_len: Hardware header length, which means that this is the *minimum size of a packet. As was pointed out, you *can* use the ax25-tools beacon program to transmit short packets! beacon does not use sendmsg(), it generates a pair of struct full_sockaddr_ax25 using libax25 ax25_aton() for source call and destination call+digipeater path, calls bind() to set the source call and then sends it with sendto(), simplified: s = socket(AF_AX25, SOCK_DGRAM, 0); len = ax25_aton(sourcecall, ); bind(s, (struct sockaddr *), len); dlen = ax25_aton(addr, ); sendto(s, message, strlen(message), 0, (struct sockaddr *), dlen); I didn't yet figure out why that works, maybe the sendto() of an AX25 datagram does not go through that hard_header_len check. If I understood things right (I'm not entirely sure about the kernel sendmsg() code path yet), there are two things that could be done here: - get the kernel fixed for supporting short raw AX.25 packet transmission again - in the mean while, change aprx to call bind() and sendto() for every packet instead of a single sendmsg() - slightly unoptimal, but at 1200 bit/s and a few packets per second, who is going to notice... - Hessu, OH7LZB