linux-kernel - Re: [Patch v7 21/22] CIFS: SMBD: Upper layer performs SMB read via RDMA write through memory registration

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: <9b02dadb-d21b-7a8d-7803-910041f66047@talpey.com>
Date:   Tue, 18 Sep 2018 22:59:50 -0700
From:   Tom Talpey <tom@...pey.com>
To:     Long Li <longli@...hange.microsoft.com>,
        Steve French <sfrench@...ba.org>, linux-cifs@...r.kernel.org,
        samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
        linux-rdma@...r.kernel.org, Christoph Hellwig <hch@...radead.org>,
        Tom Talpey <ttalpey@...rosoft.com>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>
Cc:     Long Li <longli@...rosoft.com>
Subject: Re: [Patch v7 21/22] CIFS: SMBD: Upper layer performs SMB read via
 RDMA write through memory registration

Replying to a very old message, but it's something we
discussed today at the IOLab event so to capture it:

On 11/7/2017 12:55 AM, Long Li wrote:
> From: Long Li <longli@...rosoft.com>
> 
> ---
>   fs/cifs/file.c    | 17 +++++++++++++++--
>   fs/cifs/smb2pdu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 59 insertions(+), 3 deletions(-)
> ...
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index c8afb83..8a5ff90 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2379,7 +2379,40 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
>   	req->MinimumCount = 0;
>   	req->Length = cpu_to_le32(io_parms->length);
>   	req->Offset = cpu_to_le64(io_parms->offset);
> +#ifdef CONFIG_CIFS_SMB_DIRECT
> +	/*
> +	 * If we want to do a RDMA write, fill in and append
> +	 * smbd_buffer_descriptor_v1 to the end of read request
> +	 */
> +	if (server->rdma && rdata &&
> +		rdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) {
> +
> +		struct smbd_buffer_descriptor_v1 *v1;
> +		bool need_invalidate =
> +			io_parms->tcon->ses->server->dialect == SMB30_PROT_ID;
> +
> +		rdata->mr = smbd_register_mr(
> +				server->smbd_conn, rdata->pages,
> +				rdata->nr_pages, rdata->tailsz,
> +				true, need_invalidate);
> +		if (!rdata->mr)
> +			return -ENOBUFS;
> +
> +		req->Channel = SMB2_CHANNEL_RDMA_V1_INVALIDATE;
> +		if (need_invalidate)
> +			req->Channel = SMB2_CHANNEL_RDMA_V1;
> +		req->ReadChannelInfoOffset =
> +			offsetof(struct smb2_read_plain_req, Buffer);
> +		req->ReadChannelInfoLength =
> +			sizeof(struct smbd_buffer_descriptor_v1);
> +		v1 = (struct smbd_buffer_descriptor_v1 *) &req->Buffer[0];
> +		v1->offset = rdata->mr->mr->iova;

It's unnecessary, and possibly leaking kernel information, to use
the IOVA as the offset of a memory region which is registered using
an FRWR. Because such regions are based on the exact bytes targeted
by the memory handle, the offset can be set to any value, typically
zero, but nearly arbitrary. As long as the (offset + length) does
not wrap or otherwise overflow, offset can be set to anything
convenient.

Since SMB reads and writes range up to 8MB, I'd suggest zeroing the
least significant 23 bits, which should guarantee it. The other 41
bits, party on. You could randomize them, pass some clever identifier
such as MID sequence, whatever.

Tom.

> +		v1->token = rdata->mr->mr->rkey;
> +		v1->length = rdata->mr->mr->length;