netdev - Re: [PATCH net 1/5] ibmvnic: Enforce stronger sanity checks on login response

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZM330WW6aHIQsb5y@vergenet.net>
Date: Sat, 5 Aug 2023 09:18:41 +0200
From: Simon Horman <horms@...nel.org>
To: Nick Child <nnac123@...ux.ibm.com>
Cc: netdev@...r.kernel.org, haren@...ux.ibm.com, ricklind@...ibm.com,
	danymadden@...ibm.com, tlfalcon@...ux.ibm.com,
	bjking1@...ux.ibm.com
Subject: Re: [PATCH net 1/5] ibmvnic: Enforce stronger sanity checks on login
 response

On Thu, Aug 03, 2023 at 03:20:06PM -0500, Nick Child wrote:
> Ensure that all offsets in a login response buffer are within the size
> of the allocated response buffer. Any offsets or lengths that surpass
> the allocation are likely the result of an incomplete response buffer.
> In these cases, a full reset is necessary.
> 
> When attempting to login, the ibmvnic device will allocate a response
> buffer and pass a reference to the VIOS. The VIOS will then send the
> ibmvnic device a LOGIN_RSP CRQ to signal that the buffer has been filled
> with data. If the ibmvnic device does not get a response in 20 seconds,
> the old buffer is freed and a new login request is sent. With 2
> outstanding requests, any LOGIN_RSP CRQ's could be for the older
> login request. If this is the case then the login response buffer (which
> is for the newer login request) could be incomplete and contain invalid
> data. Therefore, we must enforce strict sanity checks on the response
> buffer values.
> 
> Testing has shown that the `off_rxadd_buff_size` value is filled in last
> by the VIOS and will be the smoking gun for these circumstances.
> 
> Until VIOS can implement a mechanism for tracking outstanding response
> buffers and a method for mapping a LOGIN_RSP CRQ to a particular login
> response buffer, the best ibmvnic can do in this situation is perform a
> full reset.
> 
> Fixes: dff515a3e71d ("ibmvnic: Harden device login requests")
> Signed-off-by: Nick Child <nnac123@...ux.ibm.com>

Reviewed-by: Simon Horman <horms@...nel.org>

> ---
> 
> Hello!
> This patchset is all relevant to recent bugs which came up regarding
> the ibmvnic login process. Specifically, when this process times out.
> 
> ibmvnic devices are virtual devices which need to "login" to a physical
> NIC at the end of its initialization process. This invloves sending a
> command to the VIOS (virtual input output server, essentially the server
> that this client is logging into) requesting it to fill out a DMA mapped
> repsonse buffer. Once done, the VIOS sends a response informing the
> client that the buffer has been filled with data.
> 
> If the VIOS does not send a response in 20 seconds then the client tries
> again. If this happens then several bugs can occur. This is usually due
> to the fact that there are more than one outstanding requests and no
> mechanism for mapping a response CRQ to a given response buffer. Until
> that mechanism is created, this patchset aims to harden this timeout
> recovery process so that the device does not get stuck in an inopperable
> state.

This sort of information really belongs in a cover letter for the patchset.
And in any case, it's nice to have a cover letter if there is more
than one patch in the series.

>  drivers/net/ethernet/ibm/ibmvnic.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 763d613adbcc..996f8037c266 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -5397,6 +5397,7 @@ static int handle_login_rsp(union ibmvnic_crq *login_rsp_crq,
>  	int num_rx_pools;
>  	u64 *size_array;
>  	int i;
> +	u32 rsp_len;

nit: It's preferred in Networking code to arrange local variables in
     reverse xmas tree order - longest line to shortest.

     I know this file doesn't follow that very closely.
     But still, it would be slightly nicer, at least in this case.

...