netdev - Re: [PATCH] netpoll: Fix device name check in netpoll

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: <CAD=FV=WiS3m=dr5VrhiOPiqVL4Rb53jfEambq79GtPffiYO-Pg@mail.gmail.com>
Date:   Wed, 26 Jul 2017 11:44:58 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] netpoll: Fix device name check in netpoll_setup()

Hi,

On Tue, Jul 25, 2017 at 11:36 AM, Matthias Kaehlcke <mka@...omium.org> wrote:
> Apparently netpoll_setup() assumes that netpoll.dev_name is a pointer
> when checking if the device name is set:
>
> if (np->dev_name) {
>   ...
>
> However the field is a character array, therefore the condition always
> yields true. Check instead whether the first byte of the array has a
> non-zero value.
>
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
>  net/core/netpoll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 8357f164c660..912731bed7b7 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -666,7 +666,7 @@ int netpoll_setup(struct netpoll *np)
>         int err;
>
>         rtnl_lock();
> -       if (np->dev_name) {
> +       if (np->dev_name[0]) {
>                 struct net *net = current->nsproxy->net_ns;
>                 ndev = __dev_get_by_name(net, np->dev_name);
>         }

It's really up to the maintainer of the code, but my first instinct
here would be to instead remove the "if" test unless we really expect
dev->dev_name to be blank in lots of cases.  It will slightly slow
down the error case but should avoid an "if" test in the non-error
case.  By definition it should be safe since currently the "if" test
should always evaluate to true.

-Doug