Messages in this thread Patch in this message | | | From | Steve French <> | Date | Tue, 12 Apr 2022 17:32:03 -0500 | Subject | Re: [PATCH] cifs: potential buffer overflow in handling symlinks |
| |
Wouldn't it be easier and clearer to do the compare vs the maximum len ie
if (link_len > CIFS_MF_SYMLINK_LINK_MAXLEN)
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 >
-- Thanks,
Steve
| |