2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>: > On (23/05/06 00:11), Namjae Jeon wrote: >> From: Pumpkin <cc85nod@xxxxxxxxx> >> >> If the length of CreateContext name is larger than the tag, it will >> access >> the data following the tag and trigger KASAN global-out-of-bounds. >> >> Currently all CreateContext names are defined as string, so we can use >> strcmp instead of memcmp to avoid the out-of-bound access. Hi Chih-Yen, Please reply to Sergey's review comment. If needed, please send v2 patch after updating it. Thanks. > > [..] > >> +++ b/fs/ksmbd/oplock.c >> @@ -1492,7 +1492,7 @@ struct create_context *smb2_find_context_vals(void >> *open_req, const char *tag) >> return ERR_PTR(-EINVAL); >> >> name = (char *)cc + name_off; >> - if (memcmp(name, tag, name_len) == 0) >> + if (!strcmp(name, tag)) >> return cc; >> >> remain_len -= next; > > I'm slightly surprised that that huge `if` before memcmp() doesn't catch > it > > if ((next & 0x7) != 0 || > next > remain_len || > name_off != offsetof(struct create_context, Buffer) || > name_len < 4 || > name_off + name_len > cc_len || > (value_off & 0x7) != 0 || > (value_off && (value_off < name_off + name_len)) || > ((u64)value_off + value_len > cc_len)) > return ERR_PTR(-EINVAL); > > Is that because we should check `name_len` instead of `name_off + > name_len`? > IOW > > if (name_len != cc_len) > return ERR_PTR(); >
