| Message ID | 20230505151108.5911-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] ksmbd: fix global-out-of-bounds in smb2_find_context_vals | expand |
On (23/05/06 00:11), Namjae Jeon wrote: > From: Pumpkin <cc85nod@gmail.com> > > 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. [..] > +++ 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();
2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > On (23/05/06 00:11), Namjae Jeon wrote: >> From: Pumpkin <cc85nod@gmail.com> >> >> 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(); >
On (23/05/08 21:58), Namjae Jeon wrote: > 2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > > On (23/05/06 00:11), Namjae Jeon wrote: > >> From: Pumpkin <cc85nod@gmail.com> > >> > >> 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. Chih-Yen replied privately, but let me move the discussion back to public list. > >> +++ 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); So the question is: why doesn't this `if` catch that problem? I'd rather add one extra condition here, it doesn't make a lot of sense to strcmp/memcmp if we know beforehand that two strings have different sizes. So a simple "name len != context len" should do the trick. No?
2023-05-10 20:04 GMT+09:00, 張智諺 <cc85nod@gmail.com>: > Hi Sergey Senozhatsky, > Let me take an example to make sure what condition we should add. > > Before patching, if the tag is "ExtA" and the values of create_context > fields are following: > - next - 24 > - name_len - 8 > - name_off - 16 > In this situation, the large `if` condition will be passed: > ``` > if ((next & 0x7) != 0 || > ... > ((u64)value_off + value_len > cc_len)) > return ERR_PTR(-EINVAL); > ``` > But when we are doing `memcmp`, the ksmbd will out-of-bound access the > memory of the tag. > > However, after applying your patch, which is "name len != context len", the > large `if` condition > is still passwd, and the ksmbd still triggers the oob-read bug. > > So if we don't want to change `memcmp` to `strcmp`, what we do in the large > `if` condition is > make sure that the name length of create_context is equal or less than the > length of tag, but > we only can get the length of tag by `strlen`. > > Is my analysis correct? And do you have any ideas? Yes, we need to compare length also, And we should not use strlen() to get tag length. create context tag doesn't include null terminator. tag length is 4 or 16. We can add tag_len argument in smb2_find_context_vals(). So we change caller like this. context = smb2_find_context_vals(req, SMB2_CREATE_TIMEWARP_REQUEST, 4); and then, In the comparison part, length is also checked. if (name_len == tag_len && !memcmp(name, tag, name_len)) return cc; Thanks. > > Thanks. > > > Sergey Senozhatsky <senozhatsky@chromium.org> 於 2023年5月9日 週二 下午12:05寫道： > >> On (23/05/08 21:58), Namjae Jeon wrote: >> > 2023-05-08 10:05 GMT+09:00, Sergey Senozhatsky >> > <senozhatsky@chromium.org >> >: >> > > On (23/05/06 00:11), Namjae Jeon wrote: >> > >> From: Pumpkin <cc85nod@gmail.com> >> > >> >> > >> 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. >> >> Chih-Yen replied privately, but let me move the discussion back to >> public list. >> >> > >> +++ 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); >> >> So the question is: why doesn't this `if` catch that problem? >> I'd rather add one extra condition here, it doesn't make a lot >> of sense to strcmp/memcmp if we know beforehand that two strings >> have different sizes. So a simple "name len != context len" should >> do the trick. No? >> >
diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c index 2e54ded4d92c..5e09834016bb 100644 --- a/fs/ksmbd/oplock.c +++ 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;