netdev - Re: Sending short raw packets using sendmsg() broke

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-+m09_KS9+SyMZsTES5iLPyVE10F0u6pt4tVeYcDk8=PQ@mail.gmail.com>
Date:	Sat, 27 Feb 2016 18:02:23 -0500
From:	Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	Alan Cox <alan@...ux.intel.com>,
	Heikki Hannikainen <hessu@....iki.fi>,
	Network Development <netdev@...r.kernel.org>,
	Willem de Bruijn <willemb@...gle.com>
Subject: Re: Sending short raw packets using sendmsg() broke

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