netdev - Re: [PATCH net-next 2/2] nfp: avoid buffer leak when FW communication fails

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: <20180722.105922.88718424122435346.davem@davemloft.net>
Date:   Sun, 22 Jul 2018 10:59:22 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     jakub.kicinski@...ronome.com
Cc:     oss-drivers@...ronome.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] nfp: avoid buffer leak when FW
 communication fails

From: Jakub Kicinski <jakub.kicinski@...ronome.com>
Date: Fri, 20 Jul 2018 21:14:39 -0700

> After device is stopped we reset the rings by moving all free buffers
> to positions [0, cnt - 2], and clear the position cnt - 1 in the ring.
> We then proceed to clear the read/write pointers.  This means that if
> we try to reset the ring again the code will assume that the next to
> fill buffer is at position 0 and swap it with cnt - 1.  Since we
> previously cleared position cnt - 1 it will lead to leaking the first
> buffer and leaving ring in a bad state.
> 
> This scenario can only happen if FW communication fails, in which case
> the ring will never be used again, so the fact it's in a bad state will
> not be noticed.  Buffer leak is the only problem.  Don't try to move
> buffers in the ring if the read/write pointers indicate the ring was
> never used or have already been reset.
> 
> nfp_net_clear_config_and_disable() is now fully idempotent.
> 
> Found by code inspection, FW communication failures are very rare,
> and reconfiguring a live device is not common either, so it's unlikely
> anyone has ever noticed the leak.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@...ronome.com>

Applied.

> This is arguably net material but IMHO the risk of me missing something
> this could break is higher than the error actually occurring, and a
> page leak on a FW communication error doesn't seem like it's worth
> it at -rc6 time..  I'm happy to respin if I'm wrong!

Agreed, net-next is more appropriate for this.

Thanks.