Sending short raw packets using sendmsg() broke

Sending short raw packets using sendmsg() broke

[Heikki Hannikainen]

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 Hannikainen <hessu@hes.iki.fi>
To: Aprx software <aprx-software@googlegroups.com>
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, &src);
bind(s, (struct sockaddr *)&src, len);
dlen = ax25_aton(addr, &dest);
sendto(s, message, strlen(message), 0, (struct sockaddr *)&dest, 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

Re: Sending short raw packets using sendmsg() broke

[David Miller]

From: Heikki Hannikainen <hessu@hes.iki.fi>
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 <martin.blumenstingl@googlemail.com>
Date:   Sun Nov 22 17:46:09 2015 +0100

    packet: Allow packets with only a header (but no payload)

Re: Sending short raw packets using sendmsg() broke

[Willem de Bruijn]

> 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

[Alan Cox]

On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote:
> From: Heikki Hannikainen <hessu@hes.iki.fi>
> 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 <martin.blumenstingl@googlemail.com>
> 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

[Willem de Bruijn]

On Fri, Feb 26, 2016 at 9:44 AM, Alan Cox <alan@linux.intel.com> wrote:
> On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote:
>> From: Heikki Hannikainen <hessu@hes.iki.fi>
>> 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 <martin.blumenstingl@googlemail.com>
>> 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

[David Miller]

From: Alan Cox <alan@linux.intel.com>
Date: Fri, 26 Feb 2016 14:44:34 +0000

> On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote:
>> From: Heikki Hannikainen <hessu@hes.iki.fi>
>> 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 <martin.blumenstingl@googlemail.com>
>> 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

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
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.

Re: Sending short raw packets using sendmsg() broke

[Willem de Bruijn]

On Fri, Feb 26, 2016 at 12:46 PM, David Miller <davem@davemloft.net> wrote:
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> 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, &digi, 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

[Alan Cox]

On Fri, 2016-02-26 at 12:33 -0500, Willem de Bruijn wrote:
> On Fri, Feb 26, 2016 at 9:44 AM, Alan Cox <alan@linux.intel.com>
> wrote:
> > On Thu, 2016-02-25 at 15:26 -0500, David Miller wrote:
> > > From: Heikki Hannikainen <hessu@hes.iki.fi>
> > > 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 <martin.blumenstingl@googlemail.com>
> > > 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

[Alan Cox]

> 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, &digi,
> 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

[Willem de Bruijn]

On Tue, Mar 1, 2016 at 7:00 PM, Alan Cox <alan@linux.intel.com> 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, &digi,
>> 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

[Willem de Bruijn]

On Thu, Mar 3, 2016 at 11:40 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Tue, Mar 1, 2016 at 7:00 PM, Alan Cox <alan@linux.intel.com> 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, &digi,
>>> 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

[Alan Cox]

> > 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

[Willem de Bruijn]

On Fri, Mar 4, 2016 at 10:54 AM, Alan Cox <alan@linux.intel.com> 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.

Thanks,

  Willem

Re: Sending short raw packets using sendmsg() broke

[Willem de Bruijn]

On Fri, Mar 4, 2016 at 11:33 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, Mar 4, 2016 at 10:54 AM, Alan Cox <alan@linux.intel.com> 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