question about potential integer truncation in cifs_set_file_size

question about potential integer truncation in cifs_set_file_size

[PaX Team]

hi all,

fs/cifs/inode.c:cifs_set_file_size can truncate iattr.ia_size from loff_t
(long long) to unsigned int here:

	io_parms.length = attrs->ia_size;

and i'm wondering if this is intentional. based on a quick read of the code,
it seems that ia_size can at this point have a value over 4G (e.g., via a
call to truncate) and the ->set_file_size callback will transmit the full
64 bit value however in case of specific failures a (fallback?) call to
CIFSSMBWrite would be made with the truncated size.

FTR, this issue was detected with the upcoming version of the size overflow
plugin we have in PaX/grsecurity and there're a handful of similar cases in
the tree where potentially unwanted or unnecessary integer truncations occur,
this being one of these. any opinion/help is welcome!

cheers,
  PaX Team

Re: question about potential integer truncation in cifs_set_file_size

[Steve French]

On Sat, Sep 26, 2015 at 9:31 AM, PaX Team <pageexec-Y8qEzhMunLyT9ig0jae3mg@public.gmane.org> wrote:
> hi all,
>
> fs/cifs/inode.c:cifs_set_file_size can truncate iattr.ia_size from loff_t
> (long long) to unsigned int here:
>
>         io_parms.length = attrs->ia_size;
>
> and i'm wondering if this is intentional. based on a quick read of the code,
> it seems that ia_size can at this point have a value over 4G (e.g., via a
> call to truncate) and the ->set_file_size callback will transmit the full
> 64 bit value however in case of specific failures a (fallback?) call to
> CIFSSMBWrite would be made with the truncated size.

Good catch.  The bug here is in an error path that you probably
wouldn't notice except to very very old Windows (or DOS!) systems (ie
even before Window 2000) or
perhaps when mounted to buggy CIFS servers which don't support the
normal SetFileInfo
levels.  The actual bug is that the length and offsets fields are
backwards. Fortunately,
this code path returns immediately from write (since the length is
non-zero with a buffer
pointer of zero) without doing damage, and you wouldn't even notice
the bug since most every CIFS server supports the normal way of
setting the file size (via SetFileInfo) and
doesn't require falling back to the DOS (!) way of setting file size
by writing zero bytes
at an offset.

> FTR, this issue was detected with the upcoming version of the size overflow
> plugin we have in PaX/grsecurity and there're a handful of similar cases in
> the tree where potentially unwanted or unnecessary integer truncations occur,
> this being one of these. any opinion/help is welcome!

If you find others let me know (other than the identical bug in the
error path immediately below the one you just pointed out).

-- 
Thanks,

Steve

Re: question about potential integer truncation in cifs_set_file_size

[PaX Team]

On 28 Sep 2015 at 15:57, Steve French wrote:

> > FTR, this issue was detected with the upcoming version of the size overflow
> > plugin we have in PaX/grsecurity and there're a handful of similar cases in
> > the tree where potentially unwanted or unnecessary integer truncations occur,
> > this being one of these. any opinion/help is welcome!
> 
> If you find others let me know (other than the identical bug in the
> error path immediately below the one you just pointed out).

sure, i will, though the current (very specific) analysis found only
those questionable cases that i reported (here and some other lists).
if you (or anyone else) have ideas for more specific analysis (checking
invariants, API use, etc), we can take a look though gcc plugins have
very limited view on the source code, so only those properties can be
checked that are still visible in the middle end (we found the above
case exactly because the underlying code looks the same whether the
integer truncation is intentional or not, so we had to exclude them
as false positives in our instrumentation but still wanted to manually
verify them).

PS: it seems that you didn't CC me on the actual patch ;)