Question about dev_validate_header used in af_packet.c

Question about dev_validate_header used in af_packet.c

[Xie He]

Hi Willem,

I have a question about the function dev_validate_header used in
af_packet.c. Can you help me? Thanks!

I see when the length of the data is smaller than hard_header_len, and
when the user is "capable" enough, the function will accept it and pad
it with 0s, without validating the header with header_ops->validate.

But I think if the driver is able to accept variable-length LL
headers, shouldn't we just pass the data to header_ops->validate and
let it check the header's validity, and then just pass the validated
data to the driver for transmission?

Why when the user is "capable" enough, can it bypass the
header_ops->validate check? And why do we need to pad the data with
0s? Won't this make the driver confused about the real length of the
data?

Thank you for your help!

Xie

Re: Question about dev_validate_header used in af_packet.c

[Xie He]

On Sat, Sep 5, 2020 at 3:24 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Hi Willem,
>
> I have a question about the function dev_validate_header used in
> af_packet.c. Can you help me? Thanks!
>
> I see when the length of the data is smaller than hard_header_len, and
> when the user is "capable" enough, the function will accept it and pad
> it with 0s, without validating the header with header_ops->validate.
>
> But I think if the driver is able to accept variable-length LL
> headers, shouldn't we just pass the data to header_ops->validate and
> let it check the header's validity, and then just pass the validated
> data to the driver for transmission?
>
> Why when the user is "capable" enough, can it bypass the
> header_ops->validate check? And why do we need to pad the data with
> 0s? Won't this make the driver confused about the real length of the
> data?

Oh. I just realized that the padding of zeros won't actually make the
data longer. The padded zeros are not part of the data so the length
of the data is kept unchanged. The padding is probably because some
weird drivers are expecting this. (What drivers are them? Can we fix
them?)

I can also understand now the ability of a "capable" user to bypass
the header_ops->validate check. It is probably for testing purposes.
(Does this mean the root user will always bypass this check?)

Thanks!

Re: Question about dev_validate_header used in af_packet.c

[Willem de Bruijn]

On Sun, Sep 6, 2020 at 1:21 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Sat, Sep 5, 2020 at 3:24 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > Hi Willem,
> >
> > I have a question about the function dev_validate_header used in
> > af_packet.c. Can you help me? Thanks!
> >
> > I see when the length of the data is smaller than hard_header_len, and
> > when the user is "capable" enough, the function will accept it and pad
> > it with 0s, without validating the header with header_ops->validate.
> >
> > But I think if the driver is able to accept variable-length LL
> > headers, shouldn't we just pass the data to header_ops->validate and
> > let it check the header's validity, and then just pass the validated
> > data to the driver for transmission?
> >
> > Why when the user is "capable" enough, can it bypass the
> > header_ops->validate check? And why do we need to pad the data with
> > 0s? Won't this make the driver confused about the real length of the
> > data?
>
> Oh. I just realized that the padding of zeros won't actually make the
> data longer. The padded zeros are not part of the data so the length
> of the data is kept unchanged. The padding is probably because some
> weird drivers are expecting this. (What drivers are them? Can we fix
> them?)
>
> I can also understand now the ability of a "capable" user to bypass
> the header_ops->validate check. It is probably for testing purposes.
> (Does this mean the root user will always bypass this check?)

Apologies for the delay.

The commit that introduced the code probably summarizes state better
than I would write off the cuff:

"
commit 2793a23aacbd754dbbb5cb75093deb7e4103bace
Author: Willem de Bruijn <willemb@google.com>
Date:   Wed Mar 9 21:58:32 2016 -0500

    net: validate variable length ll headers

    Netdevice parameter hard_header_len is variously interpreted both as
    an upper and lower bound on link layer header length. The field is
    used as upper bound when reserving room at allocation, as lower bound
    when validating user input in PF_PACKET.

    Clarify the definition to be maximum header length. For validation
    of untrusted headers, add an optional validate member to header_ops.

    Allow bypassing of validation by passing CAP_SYS_RAWIO, for instance
    for deliberate testing of corrupt input. In this case, pad trailing
    bytes, as some device drivers expect completely initialized headers.

    See also http://comments.gmane.org/gmane.linux.network/401064

    Signed-off-by: Willem de Bruijn <willemb@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
"

The CAP_SYS_RAWIO exception indeed was requested to be able to
purposely test devices against bad inputs. The gmane link
unfortunately no longer works, but this was the discussion thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg99920.html

It zeroes the packet up max_header_len to ensure that an unintentional
short packet will at least not result in reading undefined data. Now
that the dust has settled around the min_header_len/max_header_len
changes, maybe now is a good time to revisit removing that
CAP_SYS_RAWIO loophole.

Re: Question about dev_validate_header used in af_packet.c

[Xie He]

On Mon, Sep 7, 2020 at 2:06 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> The CAP_SYS_RAWIO exception indeed was requested to be able to
> purposely test devices against bad inputs. The gmane link
> unfortunately no longer works, but this was the discussion thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg99920.html
>
> It zeroes the packet up max_header_len to ensure that an unintentional
> short packet will at least not result in reading undefined data. Now
> that the dust has settled around the min_header_len/max_header_len
> changes, maybe now is a good time to revisit removing that
> CAP_SYS_RAWIO loophole.

Thank you for your explanation! I can now understand the logic of
dev_hard_header. Thanks!

Do you mean we can now consider removing the ability to bypass the
header_ops->validate check? That is what I am thinking about, too!

I looked at the link you gave me. I see that Alan Cox wanted to keep
the ability of intentionally feeding corrupt frames to drivers, to
test whether drivers are able to handle incomplete headers. However, I
think after we added the header validation in af_packet.c, drivers no
longer need to ensure they can handle incomplete headers correctly
(because this is already handled by us). So there's no point in
keeping the ability to test this, either.

Re: Question about dev_validate_header used in af_packet.c

[Willem de Bruijn]

On Mon, Sep 7, 2020 at 11:17 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Mon, Sep 7, 2020 at 2:06 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > The CAP_SYS_RAWIO exception indeed was requested to be able to
> > purposely test devices against bad inputs. The gmane link
> > unfortunately no longer works, but this was the discussion thread:
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg99920.html
> >
> > It zeroes the packet up max_header_len to ensure that an unintentional
> > short packet will at least not result in reading undefined data. Now
> > that the dust has settled around the min_header_len/max_header_len
> > changes, maybe now is a good time to revisit removing that
> > CAP_SYS_RAWIO loophole.
>
> Thank you for your explanation! I can now understand the logic of
> dev_hard_header. Thanks!
>
> Do you mean we can now consider removing the ability to bypass the
> header_ops->validate check? That is what I am thinking about, too!
>
> I looked at the link you gave me. I see that Alan Cox wanted to keep
> the ability of intentionally feeding corrupt frames to drivers, to
> test whether drivers are able to handle incomplete headers. However, I
> think after we added the header validation in af_packet.c, drivers no
> longer need to ensure they can handle incomplete headers correctly
> (because this is already handled by us).

Which header validation are you referring to?

The intent is to bypass such validation to be able to test device
drivers. Note that removing that may cause someone's test to start
failing.

>  So there's no point in
> keeping the ability to test this, either.

I don't disagree in principle, but do note the failing tests. Bar any
strong reasons for change, I'd leave as is.

Re: Question about dev_validate_header used in af_packet.c

[Xie He]

On Tue, Sep 8, 2020 at 1:41 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> The intent is to bypass such validation to be able to test device
> drivers. Note that removing that may cause someone's test to start
> failing.
>
> >  So there's no point in
> > keeping the ability to test this, either.
>
> I don't disagree in principle, but do note the failing tests. Bar any
> strong reasons for change, I'd leave as is.

OK. I got what you mean. You don't want to make people's test cases fail.

I was recently looking at some drivers, and I felt that if af_packet.c
could help me filter out the invalid RAW frames, I didn't need to
check the validity of the frames myself (in the driver when
transmitting). But now I guess I still need to check that.

I feel this makes the dev_validate_header's variable-length header
check not very useful, because drivers need to do this check again
(when transmitting) anyway.

I was thinking, after I saw dev_validate_header, that we could
eventually make it completely take over the responsibility for a
driver to validate the header when transmitting RAW frames. But now it
seems we would not be able to do this.

Re: Question about dev_validate_header used in af_packet.c

[Willem de Bruijn]

On Tue, Sep 8, 2020 at 1:04 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Tue, Sep 8, 2020 at 1:41 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > The intent is to bypass such validation to be able to test device
> > drivers. Note that removing that may cause someone's test to start
> > failing.
> >
> > >  So there's no point in
> > > keeping the ability to test this, either.
> >
> > I don't disagree in principle, but do note the failing tests. Bar any
> > strong reasons for change, I'd leave as is.
>
> OK. I got what you mean. You don't want to make people's test cases fail.
>
> I was recently looking at some drivers, and I felt that if af_packet.c
> could help me filter out the invalid RAW frames, I didn't need to
> check the validity of the frames myself (in the driver when
> transmitting). But now I guess I still need to check that.
>
> I feel this makes the dev_validate_header's variable-length header
> check not very useful, because drivers need to do this check again
> (when transmitting) anyway.
>
> I was thinking, after I saw dev_validate_header, that we could
> eventually make it completely take over the responsibility for a
> driver to validate the header when transmitting RAW frames. But now it
> seems we would not be able to do this.

Agreed. As is, it is mainly useful to block users who are ns_capable,
but not capable.

A third option is to move it behind a sysctl (with static_branch). Your
point is valid that there really is no need for testing of drivers against
bad packets if the data is validated directly on kernel entry.

Re: Question about dev_validate_header used in af_packet.c

[Xie He]

On Tue, Sep 8, 2020 at 4:53 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, Sep 8, 2020 at 1:04 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > I was recently looking at some drivers, and I felt that if af_packet.c
> > could help me filter out the invalid RAW frames, I didn't need to
> > check the validity of the frames myself (in the driver when
> > transmitting). But now I guess I still need to check that.
> >
> > I feel this makes the dev_validate_header's variable-length header
> > check not very useful, because drivers need to do this check again
> > (when transmitting) anyway.
> >
> > I was thinking, after I saw dev_validate_header, that we could
> > eventually make it completely take over the responsibility for a
> > driver to validate the header when transmitting RAW frames. But now it
> > seems we would not be able to do this.
>
> Agreed. As is, it is mainly useful to block users who are ns_capable,
> but not capable.
>
> A third option is to move it behind a sysctl (with static_branch). Your
> point is valid that there really is no need for testing of drivers against
> bad packets if the data is validated directly on kernel entry.

I was thinking about this again and it came to me that maybe sometimes
people actually wanted to send invalid frames on wire (for testing the
network device on the other end of the wire)? Having thought about
this possibility I think it might be good to keep the ability for
people to have 2 choices (either having their RAW frames validated, or
not validated) through "capability" or through "sysctl" as you
mentioned. We can keep the default to be not validating the RAW frames
because RAW sockets are already intended for very special use and are
not for normal use.

Re: Question about dev_validate_header used in af_packet.c

[Willem de Bruijn]

On Tue, Sep 8, 2020 at 6:23 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Tue, Sep 8, 2020 at 4:53 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, Sep 8, 2020 at 1:04 PM Xie He <xie.he.0141@gmail.com> wrote:
> > >
> > > I was recently looking at some drivers, and I felt that if af_packet.c
> > > could help me filter out the invalid RAW frames, I didn't need to
> > > check the validity of the frames myself (in the driver when
> > > transmitting). But now I guess I still need to check that.
> > >
> > > I feel this makes the dev_validate_header's variable-length header
> > > check not very useful, because drivers need to do this check again
> > > (when transmitting) anyway.
> > >
> > > I was thinking, after I saw dev_validate_header, that we could
> > > eventually make it completely take over the responsibility for a
> > > driver to validate the header when transmitting RAW frames. But now it
> > > seems we would not be able to do this.
> >
> > Agreed. As is, it is mainly useful to block users who are ns_capable,
> > but not capable.
> >
> > A third option is to move it behind a sysctl (with static_branch). Your
> > point is valid that there really is no need for testing of drivers against
> > bad packets if the data is validated directly on kernel entry.
>
> I was thinking about this again and it came to me that maybe sometimes
> people actually wanted to send invalid frames on wire (for testing the
> network device on the other end of the wire)? Having thought about
> this possibility I think it might be good to keep the ability for
> people to have 2 choices (either having their RAW frames validated, or
> not validated) through "capability" or through "sysctl" as you
> mentioned. We can keep the default to be not validating the RAW frames
> because RAW sockets are already intended for very special use and are
> not for normal use.

That offers some configurability. But really, I would just leave it as is.