From a1e2aeb3f060c15313b271bfc66b7797c8cadf1f Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Sat, 16 Jul 2011 23:30:59 +0100 Subject: [PATCH] libafs/dir: Verify directory pathnames Provide a new routine, GetVerifiedBlob() which will ensure that the pathname contained within a directory blob is correctly terminated before returning it to the caller. For the purposes of this function, correct termination is defined as having a terminating \0 character within the same directory page as the blob itself. (cherry picked from commit d1946ffe9be0031a2daf907f5e96cf0ee7f5e15e) Change-Id: I69b9465f02417babf9b1d5179197278fac64f192 Reviewed-on: http://gerrit.openafs.org/5249 Tested-by: BuildBot Reviewed-by: Derrick Brashear --- src/afs/LINUX/osi_vnodeops.c | 21 +++--- src/afs/VNOPS/afs_vnop_readdir.c | 16 +++-- src/dir/dir.c | 114 ++++++++++++++++++++++++------- src/dir/dir.h | 2 + 4 files changed, 111 insertions(+), 42 deletions(-) diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index 7c7705ec0..4c4009434 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -264,23 +264,18 @@ afs_linux_readdir(struct file *fp, void *dirbuf, filldir_t filldir) if (!dirpos) break; - de = afs_dir_GetBlob(tdc, dirpos); - if (!de) - break; - - ino = afs_calc_inum (avc->f.fid.Fid.Volume, ntohl(de->fid.vnode)); - - if (de->name) - len = strlen(de->name); - else { - printf("afs_linux_readdir: afs_dir_GetBlob failed, null name (inode %lx, dirpos %d)\n", - (unsigned long)&tdc->f.inode, dirpos); - DRelease(de, 0); + code = afs_dir_GetVerifiedBlob(tdc, dirpos, &de); + if (code) { + afs_warn("Corrupt directory (inode %lx, dirpos %d)", + (unsigned long)&tdc->f.inode, dirpos); ReleaseSharedLock(&avc->lock); afs_PutDCache(tdc); code = -ENOENT; goto out; - } + } + + ino = afs_calc_inum (avc->f.fid.Fid.Volume, ntohl(de->fid.vnode)); + len = strlen(de->name); /* filldir returns -EINVAL when the buffer is full. */ { diff --git a/src/afs/VNOPS/afs_vnop_readdir.c b/src/afs/VNOPS/afs_vnop_readdir.c index 1c494bd9a..d123dcbd0 100644 --- a/src/afs/VNOPS/afs_vnop_readdir.c +++ b/src/afs/VNOPS/afs_vnop_readdir.c @@ -731,8 +731,12 @@ afs_readdir(OSI_VC_DECL(avc), struct uio *auio, afs_ucred_t *acred) origOffset = AFS_UIO_OFFSET(auio); /* scan for the next interesting entry scan for in-use blob otherwise up point at * this blob note that ode, if non-zero, also represents a held dir page */ - if (!(us = BlobScan(tdc, (origOffset >> 5))) - || !(nde = (struct DirEntry *)afs_dir_GetBlob(tdc, us))) { + us = BlobScan(tdc, (origOffset >> 5)); + + if (us) + afs_dir_GetVerifiedBlob(tdc, us, &nde); + + if (us == 0 || nde == NULL) { /* failed to setup nde, return what we've got, and release ode */ if (len) { /* something to hand over. */ @@ -1007,8 +1011,12 @@ afs1_readdir(struct vcache *avc, struct uio *auio, afs_ucred_t *acred) /* scan for the next interesting entry scan for in-use blob * otherwise up point at this blob note that ode, if non-zero, * also represents a held dir page */ - if (!(us = BlobScan(tdc, (origOffset >> 5))) - || !(nde = (struct DirEntry *)afs_dir_GetBlob(tdc, us))) { + us = BlobScan(tdc, (orginOffset >> 5)); + + if (us) + afs_dir_GetVerifiedBlob(tdc, us, &nde); + + if (us == 0 || nde == NULL) { /* failed to setup nde, return what we've got, and release ode */ if (len) { /* something to hand over. */ diff --git a/src/dir/dir.c b/src/dir/dir.c index b71794682..45156b6aa 100644 --- a/src/dir/dir.c +++ b/src/dir/dir.c @@ -76,6 +76,7 @@ extern void *DNew(afs_int32 *fid, int page); /* generic renaming */ #define NameBlobs afs_dir_NameBlobs #define GetBlob afs_dir_GetBlob +#define GetVerifiedBlob afs_dir_GetVerifiedBlob #define Create afs_dir_Create #define Length afs_dir_Length #define Delete afs_dir_Delete @@ -412,25 +413,23 @@ EnumerateDir(void *dir, int (*hookproc) (void *dir, char *name, num = ntohs(dhp->hashTable[i]); while (num != 0) { /* Walk down the hash table list. */ - DErrno = 0; - ep = GetBlob(dir, num); - if (!ep) { - if (DErrno) { - /* we failed, return why */ - DRelease(dhp, 0); - return DErrno; - } + code = GetVerifiedBlob(dir, num, &ep); + if (code) + goto out; + + if (!ep) break; - } num = ntohs(ep->next); code = (*hookproc) (hook, ep->name, ntohl(ep->fid.vnode), ntohl(ep->fid.vunique)); DRelease(ep, 0); if (code) - break; + goto out; } } + +out: DRelease(dhp, 0); return 0; } @@ -443,6 +442,8 @@ IsEmpty(void *dir) int num; struct DirHeader *dhp; struct DirEntry *ep; + int code; + dhp = (struct DirHeader *)DRead(dir, 0); if (!dhp) return 0; @@ -451,9 +452,13 @@ IsEmpty(void *dir) num = ntohs(dhp->hashTable[i]); while (num != 0) { /* Walk down the hash table list. */ - ep = GetBlob(dir, num); + code = GetVerifiedBlob(dir, num, &ep); + if (code) + goto out; + if (!ep) break; + if (strcmp(ep->name, "..") && strcmp(ep->name, ".")) { DRelease(ep, 0); DRelease(dhp, 0); @@ -463,19 +468,74 @@ IsEmpty(void *dir) DRelease(ep, 0); } } + +out: DRelease(dhp, 0); return 0; } +/* Return a pointer to an entry, given its number. Also return the maximum + * size of the entry, which is determined by its position within the directory + * page. + */ + +static struct DirEntry * +GetBlobWithLimit(void *dir, afs_int32 blobno, afs_size_t *maxlen) +{ + char *page; + afs_size_t pos; + + *maxlen = 0; + + page = DRead(dir, blobno >> LEPP); + if (!page) + return NULL; + + pos = 32 * (blobno & (EPP - 1)); + + *maxlen = AFS_PAGESIZE - pos - 1; + + return (struct DirEntry *)(page + pos); +} + +/* Given an entries number, return a pointer to that entry */ struct DirEntry * GetBlob(void *dir, afs_int32 blobno) { - /* Return a pointer to an entry, given its number. */ - struct DirEntry *ep; - ep = DRead(dir, blobno >> LEPP); - if (!ep) - return 0; - return (struct DirEntry *)(((long)ep) + 32 * (blobno & (EPP - 1))); + afs_size_t maxlen = 0; + return GetBlobWithLimit(dir, blobno, &maxlen); +} + +/* Return an entry, having verified that the name held within the entry + * doesn't overflow off the end of the directory page it is contained + * within + */ + +int +GetVerifiedBlob(void *file, afs_int32 blobno, struct DirEntry **outdir) +{ + struct DirEntry *dir; + afs_size_t maxlen; + char *cp; + + *outdir = NULL; + + DErrno = 0; + dir = GetBlobWithLimit(file, blobno, &maxlen); + if (!dir) + return DErrno; + + /* A blob is only valid if the name within it is NULL terminated before + * the end of the blob's containing page */ + for (cp = dir->name; *cp != '\0' && cp < ((char *)dir) + maxlen; cp++); + + if (*cp != '\0') { + DRelease(dir, 0); + return EIO; + } + + *outdir = dir; + return 0; } int @@ -512,6 +572,7 @@ FindItem(void *dir, char *ename, unsigned short **previtem) struct DirHeader *dhp; unsigned short *lp; struct DirEntry *tp; + i = DirHash(ename); dhp = (struct DirHeader *)DRead(dir, 0); if (!dhp) @@ -521,11 +582,13 @@ FindItem(void *dir, char *ename, unsigned short **previtem) DRelease(dhp, 0); return 0; } - tp = GetBlob(dir, (u_short) ntohs(dhp->hashTable[i])); - if (!tp) { + + GetVerifiedBlob(dir, (u_short) ntohs(dhp->hashTable[i]), &tp); + if (tp == NULL) { DRelease(dhp, 0); - return 0; + return NULL; } + lp = &(dhp->hashTable[i]); while (1) { /* Look at each hash conflict entry. */ @@ -541,10 +604,10 @@ FindItem(void *dir, char *ename, unsigned short **previtem) DRelease(lp, 0); /* Release all locks. */ return 0; } - tp = GetBlob(dir, (u_short) ntohs(tp->next)); - if (!tp) { + GetVerifiedBlob(dir, (u_short) ntohs(tp->next), &tp); + if (tp == NULL) { DRelease(lp, 0); - return 0; + return NULL; } } } @@ -561,11 +624,12 @@ FindFid (void *dir, afs_uint32 vnode, afs_uint32 unique) struct DirHeader *dhp; unsigned short *lp; struct DirEntry *tp; + dhp = (struct DirHeader *) DRead(dir,0); if (!dhp) return 0; for (i=0; ihashTable[i] != 0) { - tp = GetBlob(dir,(u_short)ntohs(dhp->hashTable[i])); + GetVerifiedBlob(dir,(u_short)ntohs(dhp->hashTable[i]), &tp); if (!tp) { /* should not happen */ DRelease(dhp, 0); return 0; @@ -579,7 +643,7 @@ FindFid (void *dir, afs_uint32 vnode, afs_uint32 unique) lp = &(tp->next); if (tp->next == 0) break; - tp = GetBlob(dir,(u_short)ntohs(tp->next)); + GetVerifiedBlob(dir,(u_short)ntohs(tp->next), &tp); DRelease(lp, 0); } DRelease(lp, 0); diff --git a/src/dir/dir.h b/src/dir/dir.h index d558e1ded..8fefea600 100644 --- a/src/dir/dir.h +++ b/src/dir/dir.h @@ -100,6 +100,7 @@ extern int EnumerateDir(void *dir, void *hook); extern int IsEmpty(void *dir); extern struct DirEntry *GetBlob(void *dir, afs_int32 blobno); +extern int GetVerifiedBlob(void *dir, afs_int32 blobno, struct DirEntry **); extern int DirHash(char *string); extern int DStat(int *abuffers, int *acalls, int *aios); @@ -145,6 +146,7 @@ extern int afs_dir_IsEmpty(void *dir); extern int afs_dir_ChangeFid(void *dir, char *entry, afs_uint32 *old_fid, afs_uint32 *new_fid); extern struct DirEntry *afs_dir_GetBlob(void *dir, afs_int32 blobno); +extern int afs_dir_GetVerifiedBlob(void *, afs_int32, struct DirEntry **); #endif #endif /* !defined(__AFS_DIR_H) */ -- 2.39.5