netdev - Re: [PATCH net-next v1] rtnetlink: Return error when message too short

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] [day] [month] [year] [list]

Message-ID: <m2o7omju9a.fsf@gmail.com>
Date:   Tue, 21 Mar 2023 10:04:01 +0000
From:   Donald Hunter <donald.hunter@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, donald.hunter@...hat.com
Subject: Re: [PATCH net-next v1] rtnetlink: Return error when message too short

Jakub Kicinski <kuba@...nel.org> writes:

> On Mon, 20 Mar 2023 23:18:34 +0000 Donald Hunter wrote:
>> rtnetlink_rcv_msg currently returns 0 when the message length is too
>> short. This leads to either no response at all, or an ack response
>> if NLM_F_ACK was set in the request.
>>
>> Change rtnetlink_rcv_msg to return -EINVAL which tells af_netlink to
>> generate a proper error response.
>
> It's a touch risky to start returning an error now.
> Some application could possibly have been passing an empty netlink
> message just because.

It seemed harmless enough to me, but you make a good point.

> We should give the user a heads up (pr_warn_once() with the name+pid
> of current process). Or continue returning a 0 but add a warning via
> the extack. The latter is cleaner but will not help old / sloppy apps,
> your call.

A pr_warn_once() should be enough to help the next person and also to
give visibility of any sloppy apps.

I was considering pr_warn_once() for the nlmsghdr length check in
netlink_rcv_skb since there's nowhere to reply to when the header is
malformed. 

>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>
> We can't put a Fixes tag on it. It could break uAPI, we don't want
> it backported for sure.