2022-05-05 7:46 GMT+09:00, Marios Makassikis <mmakassikis@xxxxxxxxxx>:
> The SMB2 Write packet contains data that is to be written
> to a file or to a pipe. Depending on the client, there may
> be padding between the header and the data field.
> Currently, the length is validated only in the case padding
> is present.
>
> Since the DataOffset field always points to the beginning
> of the data, there is no need to have a special case for
> padding. By removing this, the length is validated in both
> cases.
>
> Additionally, fix the length check: DataOffset and Length
> fields are relative to the SMB header start, while the packet
> length returned by get_rfc1002_len() includes 4 additional
> bytes.
>
> Signed-off-by: Marios Makassikis <mmakassikis@xxxxxxxxxx>
> ---
> Change since v2:
Hi Marios,
> - the length check was wrong, as it did not account for the rfc1002
> header in work->request_buf.
We get pdu size in ->request_buf using get_rfc1002_len(). We don't
need to account for it. So v2 patch is correct.
Thanks for your patch:)
>
> fs/ksmbd/smb2pdu.c | 49 ++++++++++++++++++----------------------------
> 1 file changed, 19 insertions(+), 30 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 16c803a9d996..23b47e505e2b 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -6328,23 +6328,18 @@ static noinline int smb2_write_pipe(struct
> ksmbd_work *work)
> length = le32_to_cpu(req->Length);
> id = req->VolatileFileId;
>
> - if (le16_to_cpu(req->DataOffset) ==
> - offsetof(struct smb2_write_req, Buffer)) {
> - data_buf = (char *)&req->Buffer[0];
> - } else {
> - if ((u64)le16_to_cpu(req->DataOffset) + length >
> - get_rfc1002_len(work->request_buf)) {
> - pr_err("invalid write data offset %u, smb_len %u\n",
> - le16_to_cpu(req->DataOffset),
> - get_rfc1002_len(work->request_buf));
> - err = -EINVAL;
> - goto out;
> - }
> -
> - data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> - le16_to_cpu(req->DataOffset));
> + if ((u64)le16_to_cpu(req->DataOffset) + length >
> + get_rfc1002_len(work->request_buf) - 4) {
> + pr_err("invalid write data offset %u, smb_len %u\n",
> + le16_to_cpu(req->DataOffset),
> + get_rfc1002_len(work->request_buf));
> + err = -EINVAL;
> + goto out;
> }
>
> + data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> + le16_to_cpu(req->DataOffset));
> +
> rpc_resp = ksmbd_rpc_write(work->sess, id, data_buf, length);
> if (rpc_resp) {
> if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
> @@ -6489,22 +6484,16 @@ int smb2_write(struct ksmbd_work *work)
>
> if (req->Channel != SMB2_CHANNEL_RDMA_V1 &&
> req->Channel != SMB2_CHANNEL_RDMA_V1_INVALIDATE) {
> - if (le16_to_cpu(req->DataOffset) ==
> - offsetof(struct smb2_write_req, Buffer)) {
> - data_buf = (char *)&req->Buffer[0];
> - } else {
> - if ((u64)le16_to_cpu(req->DataOffset) + length >
> - get_rfc1002_len(work->request_buf)) {
> - pr_err("invalid write data offset %u, smb_len %u\n",
> - le16_to_cpu(req->DataOffset),
> - get_rfc1002_len(work->request_buf));
> - err = -EINVAL;
> - goto out;
> - }
> -
> - data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> - le16_to_cpu(req->DataOffset));
> + if ((u64)le16_to_cpu(req->DataOffset) + length >
> + get_rfc1002_len(work->request_buf) - 4) {
> + pr_err("invalid write data offset %u, smb_len %u\n",
> + le16_to_cpu(req->DataOffset),
> + get_rfc1002_len(work->request_buf));
> + err = -EINVAL;
> + goto out;
> }
> + data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> + le16_to_cpu(req->DataOffset));
>
> ksmbd_debug(SMB, "flags %u\n", le32_to_cpu(req->Flags));
> if (le32_to_cpu(req->Flags) & SMB2_WRITEFLAG_WRITE_THROUGH)
> --
> 2.25.1
>
>
