Messages in this thread | | | Date | Wed, 13 Apr 2022 14:05:47 +0530 | Subject | Re: [PATCH] cifs: potential buffer overflow in handling symlinks | From | Harshit Mogalapalli <> |
| |
Hi Steve, On 13/04/22 4:02 am, Steve French wrote: > Wouldn't it be easier and clearer to do the compare vs the maximum len ie > > if (link_len > CIFS_MF_SYMLINK_LINK_MAXLEN) > Yes, thats cleaner. Will send a v2 patch.
Thanks, Harshit > instead of > > if (link_len > buf_len - CIFS_MF_SYMLINK_LINK_OFFSET) > > since buf_len is CIFS_MF_SYMLINK_FILE_SIZE and looking at link.c line > 26 and 27 this means we can use CIFS_MF_SYMLINK_LINK_OFFSET for the > comparison: > > #define CIFS_MF_SYMLINK_LINK_MAXLEN (1024) > #define CIFS_MF_SYMLINK_FILE_SIZE \ > (CIFS_MF_SYMLINK_LINK_OFFSET + CIFS_MF_SYMLINK_LINK_MAXLEN) > > On Tue, Apr 12, 2022 at 1:01 AM Harshit Mogalapalli > <harshit.m.mogalapalli@oracle.com> wrote: >> >> Smatch printed a warning: >> arch/x86/crypto/poly1305_glue.c:198 poly1305_update_arch() error: >> __memcpy() 'dctx->buf' too small (16 vs u32max) >> >> It's caused because Smatch marks 'link_len' as untrusted since it comes >> from sscanf(). Add a check to ensure that 'link_len' is not larger than >> the size of the 'link_str' buffer. >> >> Fixes: c69c1b6eaea1 ("cifs: implement CIFSParseMFSymlink()") >> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >> --- >> fs/cifs/link.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/cifs/link.c b/fs/cifs/link.c >> index 852e54e..ebfedae 100644 >> --- a/fs/cifs/link.c >> +++ b/fs/cifs/link.c >> @@ -85,6 +85,9 @@ >> if (rc != 1) >> return -EINVAL; >> >> + if (link_len > buf_len - CIFS_MF_SYMLINK_LINK_OFFSET) >> + return -EINVAL; >> + >> rc = symlink_hash(link_len, link_str, md5_hash); >> if (rc) { >> cifs_dbg(FYI, "%s: MD5 hash failure: %d\n", __func__, rc); >> -- >> 1.8.3.1 >> > >
| |