| Message ID | 20210924021254.27096-5-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | a bunch of patches that have not yet been reviewed | expand |
Looks good to me. Acked-by: Hyunchul Lee <hyc.lee@gmail.com> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > When invalid data offset and data length in request, > ksmbd_smb2_check_message check strictly and doesn't allow to process such > requests. > > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++------------------------- > 1 file changed, 37 insertions(+), 46 deletions(-) > > diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c > index 9aa46bb3e10d..697285e47522 100644 > --- a/fs/ksmbd/smb2misc.c > +++ b/fs/ksmbd/smb2misc.c > @@ -83,15 +83,18 @@ static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = { > * Returns the pointer to the beginning of the data area. Length of the data Do we need to change this comment about the return value? Looks good to me except this. Acked-by: Hyunchul Lee <hyc.lee@gmail.com> > * area and the offset to it (from the beginning of the smb are also returned. > */ > -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) > +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len, > + struct smb2_hdr *hdr) > { > + int ret = 0; > + > *off = 0; > *len = 0; > > /* error reqeusts do not have data area */ > if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED && > (((struct smb2_err_rsp *)hdr)->StructureSize) == SMB2_ERROR_STRUCTURE_SIZE2_LE) > - return NULL; > + return ret; > > /* > * Following commands have data areas so we have to get the location > @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) > case SMB2_IOCTL: > *off = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputOffset); > *len = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputCount); > - > break; > default: > ksmbd_debug(SMB, "no length check for command\n"); > break; > } > > - /* > - * Invalid length or offset probably means data area is invalid, but > - * we have little choice but to ignore the data area in this case. > - */ > if (*off > 4096) { > - ksmbd_debug(SMB, "offset %d too large, data area ignored\n", > - *off); > - *len = 0; > - *off = 0; > - } else if (*off < 0) { > - ksmbd_debug(SMB, > - "negative offset %d to data invalid ignore data area\n", > - *off); > - *off = 0; > - *len = 0; > - } else if (*len < 0) { > - ksmbd_debug(SMB, > - "negative data length %d invalid, data area ignored\n", > - *len); > - *len = 0; > - } else if (*len > 128 * 1024) { > - ksmbd_debug(SMB, "data area larger than 128K: %d\n", *len); > - *len = 0; > + ksmbd_debug(SMB, "offset %d too large\n", *off); > + ret = -EINVAL; > + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) { > + ksmbd_debug(SMB, "Request is larger than maximum stream protocol length(%u): %llu\n", > + MAX_STREAM_PROT_LEN, (u64)*off + *len); > + ret = -EINVAL; > } > > - /* return pointer to beginning of data area, ie offset from SMB start */ > - if ((*off != 0) && (*len != 0)) > - return (char *)hdr + *off; > - else > - return NULL; > + return ret; > } > > /* > * Calculate the size of the SMB message based on the fixed header > * portion, the number of word parameters and the data portion of the message. > */ > -static unsigned int smb2_calc_size(void *buf) > +static int smb2_calc_size(void *buf, unsigned int *len) > { > struct smb2_pdu *pdu = (struct smb2_pdu *)buf; > struct smb2_hdr *hdr = &pdu->hdr; > - int offset; /* the offset from the beginning of SMB to data area */ > - int data_length; /* the length of the variable length data area */ > + unsigned int offset; /* the offset from the beginning of SMB to data area */ > + unsigned int data_length; /* the length of the variable length data area */ > + int ret; > + > /* Structure Size has already been checked to make sure it is 64 */ > - int len = le16_to_cpu(hdr->StructureSize); > + *len = le16_to_cpu(hdr->StructureSize); > > /* > * StructureSize2, ie length of fixed parameter area has already > * been checked to make sure it is the correct length. > */ > - len += le16_to_cpu(pdu->StructureSize2); > + *len += le16_to_cpu(pdu->StructureSize2); > > if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false) > goto calc_size_exit; > > - smb2_get_data_area_len(&offset, &data_length, hdr); > - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length, > + ret = smb2_get_data_area_len(&offset, &data_length, hdr); > + if (ret) > + return ret; > + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length, > offset); > > if (data_length > 0) { > @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf) > * for some commands, typically those with odd StructureSize, > * so we must add one to the calculation. > */ > - if (offset + 1 < len) > + if (offset + 1 < *len) { > ksmbd_debug(SMB, > - "data area offset %d overlaps SMB2 header %d\n", > - offset + 1, len); > - else > - len = offset + data_length; > + "data area offset %d overlaps SMB2 header %u\n", > + offset + 1, *len); > + return -EINVAL; > + } > + > + *len = offset + data_length; > } > + > calc_size_exit: > - ksmbd_debug(SMB, "SMB2 len %d\n", len); > - return len; > + ksmbd_debug(SMB, "SMB2 len %u\n", *len); > + return 0; > } > > static inline int smb2_query_info_req_len(struct smb2_query_info_req *h) > @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) > return 1; > } > > - clc_len = smb2_calc_size(hdr); > + if (smb2_calc_size(hdr, &clc_len)) > + return 1; > + > if (len != clc_len) { > - /* server can return one byte more due to implied bcc[0] */ > + /* client can return one byte more due to implied bcc[0] */ > if (clc_len == len + 1) > return 0; > > -- > 2.25.1 > -- Thanks, Hyunchul
2021-09-25 19:27 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > Looks good to me. > Acked-by: Hyunchul Lee <hyc.lee@gmail.com> > > 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> When invalid data offset and data length in request, >> ksmbd_smb2_check_message check strictly and doesn't allow to process such >> requests. >> >> Cc: Tom Talpey <tom@talpey.com> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Cc: Hyunchul Lee <hyc.lee@gmail.com> >> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++------------------------- >> 1 file changed, 37 insertions(+), 46 deletions(-) >> >> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c >> index 9aa46bb3e10d..697285e47522 100644 >> --- a/fs/ksmbd/smb2misc.c >> +++ b/fs/ksmbd/smb2misc.c >> @@ -83,15 +83,18 @@ static const bool >> has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = { >> * Returns the pointer to the beginning of the data area. Length of the >> data > > Do we need to change this comment about the return value? Will update it on next version. > Looks good to me except this. > Acked-by: Hyunchul Lee <hyc.lee@gmail.com> Thanks! > > >> * area and the offset to it (from the beginning of the smb are also >> returned. >> */ >> -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr >> *hdr) >> +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len, >> + struct smb2_hdr *hdr) >> { >> + int ret = 0; >> + >> *off = 0; >> *len = 0; >> >> /* error reqeusts do not have data area */ >> if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED >> && >> (((struct smb2_err_rsp *)hdr)->StructureSize) == >> SMB2_ERROR_STRUCTURE_SIZE2_LE) >> - return NULL; >> + return ret; >> >> /* >> * Following commands have data areas so we have to get the >> location >> @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int >> *len, struct smb2_hdr *hdr) >> case SMB2_IOCTL: >> *off = le32_to_cpu(((struct smb2_ioctl_req >> *)hdr)->InputOffset); >> *len = le32_to_cpu(((struct smb2_ioctl_req >> *)hdr)->InputCount); >> - >> break; >> default: >> ksmbd_debug(SMB, "no length check for command\n"); >> break; >> } >> >> - /* >> - * Invalid length or offset probably means data area is invalid, >> but >> - * we have little choice but to ignore the data area in this >> case. >> - */ >> if (*off > 4096) { >> - ksmbd_debug(SMB, "offset %d too large, data area >> ignored\n", >> - *off); >> - *len = 0; >> - *off = 0; >> - } else if (*off < 0) { >> - ksmbd_debug(SMB, >> - "negative offset %d to data invalid ignore >> data area\n", >> - *off); >> - *off = 0; >> - *len = 0; >> - } else if (*len < 0) { >> - ksmbd_debug(SMB, >> - "negative data length %d invalid, data area >> ignored\n", >> - *len); >> - *len = 0; >> - } else if (*len > 128 * 1024) { >> - ksmbd_debug(SMB, "data area larger than 128K: %d\n", >> *len); >> - *len = 0; >> + ksmbd_debug(SMB, "offset %d too large\n", *off); >> + ret = -EINVAL; >> + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) { >> + ksmbd_debug(SMB, "Request is larger than maximum stream >> protocol length(%u): %llu\n", >> + MAX_STREAM_PROT_LEN, (u64)*off + *len); >> + ret = -EINVAL; >> } >> >> - /* return pointer to beginning of data area, ie offset from SMB >> start */ >> - if ((*off != 0) && (*len != 0)) >> - return (char *)hdr + *off; >> - else >> - return NULL; >> + return ret; >> } >> >> /* >> * Calculate the size of the SMB message based on the fixed header >> * portion, the number of word parameters and the data portion of the >> message. >> */ >> -static unsigned int smb2_calc_size(void *buf) >> +static int smb2_calc_size(void *buf, unsigned int *len) >> { >> struct smb2_pdu *pdu = (struct smb2_pdu *)buf; >> struct smb2_hdr *hdr = &pdu->hdr; >> - int offset; /* the offset from the beginning of SMB to data area >> */ >> - int data_length; /* the length of the variable length data area >> */ >> + unsigned int offset; /* the offset from the beginning of SMB to >> data area */ >> + unsigned int data_length; /* the length of the variable length >> data area */ >> + int ret; >> + >> /* Structure Size has already been checked to make sure it is 64 >> */ >> - int len = le16_to_cpu(hdr->StructureSize); >> + *len = le16_to_cpu(hdr->StructureSize); >> >> /* >> * StructureSize2, ie length of fixed parameter area has already >> * been checked to make sure it is the correct length. >> */ >> - len += le16_to_cpu(pdu->StructureSize2); >> + *len += le16_to_cpu(pdu->StructureSize2); >> >> if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false) >> goto calc_size_exit; >> >> - smb2_get_data_area_len(&offset, &data_length, hdr); >> - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length, >> + ret = smb2_get_data_area_len(&offset, &data_length, hdr); >> + if (ret) >> + return ret; >> + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length, >> offset); >> >> if (data_length > 0) { >> @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf) >> * for some commands, typically those with odd >> StructureSize, >> * so we must add one to the calculation. >> */ >> - if (offset + 1 < len) >> + if (offset + 1 < *len) { >> ksmbd_debug(SMB, >> - "data area offset %d overlaps SMB2 >> header %d\n", >> - offset + 1, len); >> - else >> - len = offset + data_length; >> + "data area offset %d overlaps SMB2 >> header %u\n", >> + offset + 1, *len); >> + return -EINVAL; >> + } >> + >> + *len = offset + data_length; >> } >> + >> calc_size_exit: >> - ksmbd_debug(SMB, "SMB2 len %d\n", len); >> - return len; >> + ksmbd_debug(SMB, "SMB2 len %u\n", *len); >> + return 0; >> } >> >> static inline int smb2_query_info_req_len(struct smb2_query_info_req *h) >> @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work >> *work) >> return 1; >> } >> >> - clc_len = smb2_calc_size(hdr); >> + if (smb2_calc_size(hdr, &clc_len)) >> + return 1; >> + >> if (len != clc_len) { >> - /* server can return one byte more due to implied bcc[0] >> */ >> + /* client can return one byte more due to implied bcc[0] >> */ >> if (clc_len == len + 1) >> return 0; >> >> -- >> 2.25.1 >> > > > -- > Thanks, > Hyunchul >
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c index 9aa46bb3e10d..697285e47522 100644 --- a/fs/ksmbd/smb2misc.c +++ b/fs/ksmbd/smb2misc.c @@ -83,15 +83,18 @@ static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = { * Returns the pointer to the beginning of the data area. Length of the data * area and the offset to it (from the beginning of the smb are also returned. */ -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len, + struct smb2_hdr *hdr) { + int ret = 0; + *off = 0; *len = 0; /* error reqeusts do not have data area */ if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED && (((struct smb2_err_rsp *)hdr)->StructureSize) == SMB2_ERROR_STRUCTURE_SIZE2_LE) - return NULL; + return ret; /* * Following commands have data areas so we have to get the location @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) case SMB2_IOCTL: *off = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputOffset); *len = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputCount); - break; default: ksmbd_debug(SMB, "no length check for command\n"); break; } - /* - * Invalid length or offset probably means data area is invalid, but - * we have little choice but to ignore the data area in this case. - */ if (*off > 4096) { - ksmbd_debug(SMB, "offset %d too large, data area ignored\n", - *off); - *len = 0; - *off = 0; - } else if (*off < 0) { - ksmbd_debug(SMB, - "negative offset %d to data invalid ignore data area\n", - *off); - *off = 0; - *len = 0; - } else if (*len < 0) { - ksmbd_debug(SMB, - "negative data length %d invalid, data area ignored\n", - *len); - *len = 0; - } else if (*len > 128 * 1024) { - ksmbd_debug(SMB, "data area larger than 128K: %d\n", *len); - *len = 0; + ksmbd_debug(SMB, "offset %d too large\n", *off); + ret = -EINVAL; + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) { + ksmbd_debug(SMB, "Request is larger than maximum stream protocol length(%u): %llu\n", + MAX_STREAM_PROT_LEN, (u64)*off + *len); + ret = -EINVAL; } - /* return pointer to beginning of data area, ie offset from SMB start */ - if ((*off != 0) && (*len != 0)) - return (char *)hdr + *off; - else - return NULL; + return ret; } /* * Calculate the size of the SMB message based on the fixed header * portion, the number of word parameters and the data portion of the message. */ -static unsigned int smb2_calc_size(void *buf) +static int smb2_calc_size(void *buf, unsigned int *len) { struct smb2_pdu *pdu = (struct smb2_pdu *)buf; struct smb2_hdr *hdr = &pdu->hdr; - int offset; /* the offset from the beginning of SMB to data area */ - int data_length; /* the length of the variable length data area */ + unsigned int offset; /* the offset from the beginning of SMB to data area */ + unsigned int data_length; /* the length of the variable length data area */ + int ret; + /* Structure Size has already been checked to make sure it is 64 */ - int len = le16_to_cpu(hdr->StructureSize); + *len = le16_to_cpu(hdr->StructureSize); /* * StructureSize2, ie length of fixed parameter area has already * been checked to make sure it is the correct length. */ - len += le16_to_cpu(pdu->StructureSize2); + *len += le16_to_cpu(pdu->StructureSize2); if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false) goto calc_size_exit; - smb2_get_data_area_len(&offset, &data_length, hdr); - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length, + ret = smb2_get_data_area_len(&offset, &data_length, hdr); + if (ret) + return ret; + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length, offset); if (data_length > 0) { @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf) * for some commands, typically those with odd StructureSize, * so we must add one to the calculation. */ - if (offset + 1 < len) + if (offset + 1 < *len) { ksmbd_debug(SMB, - "data area offset %d overlaps SMB2 header %d\n", - offset + 1, len); - else - len = offset + data_length; + "data area offset %d overlaps SMB2 header %u\n", + offset + 1, *len); + return -EINVAL; + } + + *len = offset + data_length; } + calc_size_exit: - ksmbd_debug(SMB, "SMB2 len %d\n", len); - return len; + ksmbd_debug(SMB, "SMB2 len %u\n", *len); + return 0; } static inline int smb2_query_info_req_len(struct smb2_query_info_req *h) @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) return 1; } - clc_len = smb2_calc_size(hdr); + if (smb2_calc_size(hdr, &clc_len)) + return 1; + if (len != clc_len) { - /* server can return one byte more due to implied bcc[0] */ + /* client can return one byte more due to implied bcc[0] */ if (clc_len == len + 1) return 0;
When invalid data offset and data length in request, ksmbd_smb2_check_message check strictly and doesn't allow to process such requests. Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Cc: Hyunchul Lee <hyc.lee@gmail.com> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 46 deletions(-)