| Message ID | 20220504224640.2865787-1-mmakassikis@freebox.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ksmbd: validate length in smb2_write() | expand |
2022-05-05 7:46 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>: > 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@freebox.fr> > --- > 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 > >
Hello Marios, 2022년 5월 5일 (목) 오후 4:00, Marios Makassikis <mmakassikis@freebox.fr>님이 작성: > > 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. > get_rfc1002_len doesn't include additional 4 bytes. Can you check it again? > Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr> > --- > Change since v2: > - the length check was wrong, as it did not account for the rfc1002 > header in work->request_buf. > > 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 > -- Thanks, Hyunchul
On Fri, May 6, 2022 at 1:42 AM Hyunchul Lee <hyc.lee@gmail.com> wrote: > > Hello Marios, > > 2022년 5월 5일 (목) 오후 4:00, Marios Makassikis <mmakassikis@freebox.fr>님이 작성: > > > > > 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. > > > > get_rfc1002_len doesn't include additional 4 bytes. > Can you check it again? > Hello Hyunchul, Namjae, You are both right, v3 is not necessary. My confusion came from looking at earlier code (without the changes to smb2_hdr) rather than the latest tree. Thanks for checking it. Marios
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)
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@freebox.fr> --- Change since v2: - the length check was wrong, as it did not account for the rfc1002 header in work->request_buf. fs/ksmbd/smb2pdu.c | 49 ++++++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 30 deletions(-)