| Message ID | 20211002131212.130629-2-slow@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Buffer validation patches | expand |
2021-10-02 22:11 GMT+09:00, Ralph Boehme <slow@samba.org>: > From: Namjae Jeon <linkinjeon@kernel.org> > > This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol > length exceeds maximum value. opencode pdu size check in > ksmbd_pdu_size_has_room(). > > 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: Sergey Senozhatsky <senozhatsky@chromium.org> > Acked-by: Hyunchul Lee <hyc.lee@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/ksmbd/connection.c | 9 +++++---- > fs/ksmbd/smb_common.c | 6 ------ > fs/ksmbd/smb_common.h | 4 ++-- > 3 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c > index af086d35398a..e50353c50661 100644 > --- a/fs/ksmbd/connection.c > +++ b/fs/ksmbd/connection.c > @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p) > pdu_size = get_rfc1002_len(hdr_buf); > ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size); > > - /* make sure we have enough to get to SMB header end */ > - if (!ksmbd_pdu_size_has_room(pdu_size)) { > - ksmbd_debug(CONN, "SMB request too short (%u bytes)\n", > - pdu_size); > + /* > + * Check if pdu size is valid (min : smb header size, > + * max : 0x00FFFFFF). > + */ > + if (pdu_size > MAX_STREAM_PROT_LEN) { You changed this patch not to check header size check. I think that this patch should be backed to original one. Because your patches are clean-up patches, not fixes. So you need to remove header size check here in "ksmbd: check PDU len is at least header plus body size... ". > continue; > } > > diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c > index db8042a173d0..b6c4c7e960fa 100644 > --- a/fs/ksmbd/smb_common.c > +++ b/fs/ksmbd/smb_common.c > @@ -21,7 +21,6 @@ static const char basechars[43] = > "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%"; > #define MAGIC_CHAR '~' > #define PERIOD '.' > #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE])) > -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr)) > > struct smb_protocol { > int index; > @@ -294,11 +293,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work) > return 0; > } > > -bool ksmbd_pdu_size_has_room(unsigned int pdu) > -{ > - return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4); > -} > - > int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int > info_level, > struct ksmbd_file *dir, > struct ksmbd_dir_info *d_info, > diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h > index 994abede27e9..6e79e7577f6b 100644 > --- a/fs/ksmbd/smb_common.h > +++ b/fs/ksmbd/smb_common.h > @@ -48,6 +48,8 @@ > #define CIFS_DEFAULT_IOSIZE (64 * 1024) > #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */ > > +#define MAX_STREAM_PROT_LEN 0x00FFFFFF > + > /* Responses when opening a file. */ > #define F_SUPERSEDED 0 > #define F_OPENED 1 > @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, > __le16 dialects_count); > > int ksmbd_init_smb_server(struct ksmbd_work *work); > > -bool ksmbd_pdu_size_has_room(unsigned int pdu); > - > struct ksmbd_kstat; > int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, > int info_level, > -- > 2.31.1 > >
Am 03.10.21 um 03:18 schrieb Namjae Jeon: > 2021-10-02 22:11 GMT+09:00, Ralph Boehme <slow@samba.org>: >> From: Namjae Jeon <linkinjeon@kernel.org> >> >> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol >> length exceeds maximum value. opencode pdu size check in >> ksmbd_pdu_size_has_room(). >> >> 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: Sergey Senozhatsky <senozhatsky@chromium.org> >> Acked-by: Hyunchul Lee <hyc.lee@gmail.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/ksmbd/connection.c | 9 +++++---- >> fs/ksmbd/smb_common.c | 6 ------ >> fs/ksmbd/smb_common.h | 4 ++-- >> 3 files changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c >> index af086d35398a..e50353c50661 100644 >> --- a/fs/ksmbd/connection.c >> +++ b/fs/ksmbd/connection.c >> @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p) >> pdu_size = get_rfc1002_len(hdr_buf); >> ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size); >> >> - /* make sure we have enough to get to SMB header end */ >> - if (!ksmbd_pdu_size_has_room(pdu_size)) { >> - ksmbd_debug(CONN, "SMB request too short (%u bytes)\n", >> - pdu_size); >> + /* >> + * Check if pdu size is valid (min : smb header size, >> + * max : 0x00FFFFFF). >> + */ >> + if (pdu_size > MAX_STREAM_PROT_LEN) { > You changed this patch not to check header size check. yes, on purpose. Because I though that your change was wrong and the whole logic should be done differently. Unfortunately I missed the fact that there are a bunch of other places where header data is accessed early, before reaching ksmbd_smb2_check_message() where I wanted to consolidate the checks. > I think that > this patch should be backed to original one. > Because your patches are > clean-up patches, not fixes. there are two more invalid reads that the patchset address. But agreed, I will now simply work my patches on-top of yours. Thanks! -slow
diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c index af086d35398a..e50353c50661 100644 --- a/fs/ksmbd/connection.c +++ b/fs/ksmbd/connection.c @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p) pdu_size = get_rfc1002_len(hdr_buf); ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size); - /* make sure we have enough to get to SMB header end */ - if (!ksmbd_pdu_size_has_room(pdu_size)) { - ksmbd_debug(CONN, "SMB request too short (%u bytes)\n", - pdu_size); + /* + * Check if pdu size is valid (min : smb header size, + * max : 0x00FFFFFF). + */ + if (pdu_size > MAX_STREAM_PROT_LEN) { continue; } diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c index db8042a173d0..b6c4c7e960fa 100644 --- a/fs/ksmbd/smb_common.c +++ b/fs/ksmbd/smb_common.c @@ -21,7 +21,6 @@ static const char basechars[43] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%"; #define MAGIC_CHAR '~' #define PERIOD '.' #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE])) -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr)) struct smb_protocol { int index; @@ -294,11 +293,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work) return 0; } -bool ksmbd_pdu_size_has_room(unsigned int pdu) -{ - return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4); -} - int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level, struct ksmbd_file *dir, struct ksmbd_dir_info *d_info, diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h index 994abede27e9..6e79e7577f6b 100644 --- a/fs/ksmbd/smb_common.h +++ b/fs/ksmbd/smb_common.h @@ -48,6 +48,8 @@ #define CIFS_DEFAULT_IOSIZE (64 * 1024) #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */ +#define MAX_STREAM_PROT_LEN 0x00FFFFFF + /* Responses when opening a file. */ #define F_SUPERSEDED 0 #define F_OPENED 1 @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count); int ksmbd_init_smb_server(struct ksmbd_work *work); -bool ksmbd_pdu_size_has_room(unsigned int pdu); - struct ksmbd_kstat; int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,