potential out-of-bounds memory write in afs_alloc_cell()

[Colin Ian King]

Hi there,

static analysis with coverity has detected a potential out-of-bounds
memory write in afs_alloc_cell():

Analysis is as follows:


129
   1. Condition !!!name, taking false branch.
   2. Condition !!!name, taking false branch.
130        ASSERT(name);

   3. Condition namelen == 0, taking false branch.
   4. cond_at_least: Checking namelen == 0U implies that namelen is at
least 1 on the false branch.
131        if (namelen == 0)
132                return ERR_PTR(-EINVAL);

   5. Condition namelen > 256, taking false branch.
   6. cond_between: Checking namelen > 256U implies that namelen is
between 1 and 256 (inclusive) on the false branch.
133        if (namelen > AFS_MAXCELLNAME) {
134                _leave(" = -ENAMETOOLONG");
135                return ERR_PTR(-ENAMETOOLONG);
136        }
137
138        /* Prohibit cell names that contain unprintable chars, '/'
and '@' or
139         * that begin with a dot.  This also precludes "@cell".
140         */

   7. Condition name[0] == '.', taking false branch.
141        if (name[0] == '.')
142                return ERR_PTR(-EINVAL);
   8. Condition i < namelen, taking true branch.
   13. Condition i < namelen, taking true branch.
   18. Condition i < namelen, taking false branch.

143        for (i = 0; i < namelen; i++) {
144                char ch = name[i];

   9. Condition !((_ctype[(int)(unsigned char)ch] & (151 /* (((0x10 | 1)
| 2) | 4) | 0x80 */)) != 0), taking false branch.
   10. Condition ch == '/', taking false branch.
   11. Condition ch == '@', taking false branch.
   14. Condition !((_ctype[(int)(unsigned char)ch] & (151 /* (((0x10 |
1) | 2) | 4) | 0x80 */)) != 0), taking false branch.
   15. Condition ch == '/', taking false branch.
   16. Condition ch == '@', taking false branch.
145                if (!isprint(ch) || ch == '/' || ch == '@')
146                        return ERR_PTR(-EINVAL);

   12. Jumping back to the beginning of the loop.
   17. Jumping back to the beginning of the loop.
147        }
148

   19. Condition !!(afs_debug & 1), taking true branch.
   20. Condition !!(afs_debug & 1), taking true branch.
149        _enter("%*.*s,%s", namelen, namelen, name, addresses);
150
151        cell = kzalloc(sizeof(struct afs_cell), GFP_KERNEL);

   21. Condition !cell, taking false branch.
152        if (!cell) {
153                _leave(" = -ENOMEM");
154                return ERR_PTR(-ENOMEM);
155        }
156
157        cell->net = net;
158        cell->name_len = namelen;

   22. Condition i < namelen, taking true branch.
   24. Condition i < namelen, taking true branch.
   25. cond_at_most: Checking i < namelen implies that i may be up to
255 on the true branch.
159        for (i = 0; i < namelen; i++)

   23. Jumping back to the beginning of the loop.

CID (#1 of 1): Out-of-bounds write (OVERRUN)
   26. overrun-local: Overrunning array cell->name of 65 bytes at byte
offset 255 using index i (which evaluates to 255).
160                cell->name[i] = tolower(name[i]);

The above write to cell->names[i] occurs because it is declared as a 64
byte char array in fs/afs/internal.h:

char                    name[64 + 1];   /* Cell name, case-flattened and
NUL-padded */

however we may be indexing beyond this. I was unsure if name should be
AFS_MAXCELLNAME + 1 instead of the hard coded size here, or if this
situation can't happen and the 65 char limit is intentional.

Colin