netdev - Re: [PATCH net-next 00/14][pull request] ice: refactor mailbox overflow detection

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: <20230315090152.GS36557@unreal>
Date:   Wed, 15 Mar 2023 11:01:52 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 00/14][pull request] ice: refactor mailbox
 overflow detection

On Tue, Mar 14, 2023 at 02:26:10PM -0700, Jacob Keller wrote:
> 
> 
> On 3/14/2023 6:57 AM, Leon Romanovsky wrote:
> > On Mon, Mar 13, 2023 at 11:21:09AM -0700, Tony Nguyen wrote:
> >> Jake Keller says:
> >>
> >> The primary motivation of this series is to cleanup and refactor the mailbox
> >> overflow detection logic such that it will work with Scalable IOV. In
> >> addition a few other minor cleanups are done while I was working on the
> >> code in the area.
> >>
> >> First, the mailbox overflow functions in ice_vf_mbx.c are refactored to
> >> store the data per-VF as an embedded structure in struct ice_vf, rather than
> >> stored separately as a fixed-size array which only works with Single Root
> >> IOV. This reduces the overall memory footprint when only a handful of VFs
> >> are used.
> >>
> >> The overflow detection functions are also cleaned up to reduce the need for
> >> multiple separate calls to determine when to report a VF as potentially
> >> malicious.
> >>
> >> Finally, the ice_is_malicious_vf function is cleaned up and moved into
> >> ice_virtchnl.c since it is not Single Root IOV specific, and thus does not
> >> belong in ice_sriov.c
> >>
> >> I could probably have done this in fewer patches, but I split pieces out to
> >> hopefully aid in reviewing the overall sequence of changes. This does cause
> >> some additional thrash as it results in intermediate versions of the
> >> refactor, but I think its worth it for making each step easier to
> >> understand.
> >>
> >> The following are changes since commit 95b744508d4d5135ae2a096ff3f0ee882bcc52b3:
> >>   qede: remove linux/version.h and linux/compiler.h
> >> and are available in the git repository at:
> >>   git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE
> >>
> >> Jacob Keller (14):
> >>   ice: re-order ice_mbx_reset_snapshot function
> >>   ice: convert ice_mbx_clear_malvf to void and use WARN
> >>   ice: track malicious VFs in new ice_mbx_vf_info structure
> >>   ice: move VF overflow message count into struct ice_mbx_vf_info
> >>   ice: remove ice_mbx_deinit_snapshot
> >>   ice: merge ice_mbx_report_malvf with ice_mbx_vf_state_handler
> >>   ice: initialize mailbox snapshot earlier in PF init
> >>   ice: declare ice_vc_process_vf_msg in ice_virtchnl.h
> >>   ice: always report VF overflowing mailbox even without PF VSI
> >>   ice: remove unnecessary &array[0] and just use array
> >>   ice: pass mbxdata to ice_is_malicious_vf()
> >>   ice: print message if ice_mbx_vf_state_handler returns an error
> >>   ice: move ice_is_malicious_vf() to ice_virtchnl.c
> >>   ice: call ice_is_malicious_vf() from ice_vc_process_vf_msg()
> > 
> > Everything looks legit except your anti-spamming logic which IMHO
> > shouldn't happen in first place.
> > 
> 
> Without the checks there's no warning to the system administrator that a
> VM may have been misconfigured or modified to spam messages. If this
> occurs, the VM can overload the PF's mailbox queue and prevent other VFs
> from using the queue normally, and thus performing a denial of service.
> 
> My understanding (I was not involved in the original implementation or
> discussions) is that there is no hardware mechanism to prevent such
> overflow in this device. This is an oversight in the design which was
> not caught until it was too late to make such a change.
> 
> The original checks were added in 0891c89674e8 ("ice: warn about
> potentially malicious VFs"), but it seems that commit message did not
> provide much detail :(

Thanks for the explanation.

> 
> -Jake
> 
> > Thanks,
> > Reviewed-by: Leon Romanovsky <leonro@...dia.com>