| Message ID | 20170801120438.1582336-2-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ext4: fix warning about stack corruption | expand |
On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we > get a warning in adfs about a possible buffer overflow: > > In function 'memcpy', > inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2, > inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2: > include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter > __read_overflow2(); > ^~~~~~~~~~~~~~~~~~ > > The warning is correct in the sense that a negative 'pos' argument > to the function would have that result. However, this is not a bug, > as we know the position is always positive (in fact, between 5 > and 2007, inclusive) when the function gets called. > > Changing the variable to a unsigned type avoids the problem. I decided > to use 'unsigned int' for the position in the directory and the block > number, as they are both counting things, but use size_t for the > offset and length that get passed into memcpy. This shuts up the > warning. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Kees Cook <keescook@chromium.org> Thanks for the fix! (Added sfr to Cc since he noticed this too.) -Kees -- Kees Cook Pixel Security
Hi Arnd, On Tue, 1 Aug 2017 11:20:26 -0700 Kees Cook <keescook@chromium.org> wrote: > > On Tue, Aug 1, 2017 at 5:04 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we > > get a warning in adfs about a possible buffer overflow: > > > > In function 'memcpy', > > inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2, > > inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2: > > include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter > > __read_overflow2(); > > ^~~~~~~~~~~~~~~~~~ > > > > The warning is correct in the sense that a negative 'pos' argument > > to the function would have that result. However, this is not a bug, > > as we know the position is always positive (in fact, between 5 > > and 2007, inclusive) when the function gets called. > > > > Changing the variable to a unsigned type avoids the problem. I decided > > to use 'unsigned int' for the position in the directory and the block > > number, as they are both counting things, but use size_t for the > > offset and length that get passed into memcpy. This shuts up the > > warning. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Acked-by: Kees Cook <keescook@chromium.org> > > Thanks for the fix! (Added sfr to Cc since he noticed this too.) Can someone please send me the patch so I can use it if Andrew doesn't get around to updating mmotd today? -- Cheers, Stephen Rothwell
diff --git a/fs/adfs/dir_f.c b/fs/adfs/dir_f.c index 0fbfd0b04ae0..dab3595a1ecc 100644 --- a/fs/adfs/dir_f.c +++ b/fs/adfs/dir_f.c @@ -283,11 +283,12 @@ __adfs_dir_get(struct adfs_dir *dir, int pos, struct object_info *obj) } static int -__adfs_dir_put(struct adfs_dir *dir, int pos, struct object_info *obj) +__adfs_dir_put(struct adfs_dir *dir, unsigned int pos, struct object_info *obj) { struct super_block *sb = dir->sb; struct adfs_direntry de; - int thissize, buffer, offset; + unsigned int buffer; + size_t thissize, offset; buffer = pos >> sb->s_blocksize_bits;
After commit 62d1034f53e3 ("fortify: use WARN instead of BUG for now"), we get a warning in adfs about a possible buffer overflow: In function 'memcpy', inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2, inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2: include/linux/string.h:305:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter __read_overflow2(); ^~~~~~~~~~~~~~~~~~ The warning is correct in the sense that a negative 'pos' argument to the function would have that result. However, this is not a bug, as we know the position is always positive (in fact, between 5 and 2007, inclusive) when the function gets called. Changing the variable to a unsigned type avoids the problem. I decided to use 'unsigned int' for the position in the directory and the block number, as they are both counting things, but use size_t for the offset and length that get passed into memcpy. This shuts up the warning. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/adfs/dir_f.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.9.0