Hello. I've found a small bug in what appears to be the maximum file
size handling code.
The problem here, as far as I understand, is that casting from an
unsigned type to a signed type, when the latter cannot represent the
arithmetic value of the former, is UB. In practice, under the PaX size
overflow protection, this code crashes when mounting from FreeBSD
servers that send "all ones" in the size field.
Not sure I'm doing things right with the list and I'm not subscribed, so
please CC.
The fix should look something like this:
--- fs/nfs/internal.h.orig 2015-11-02 10:05:25.000000000 +1000
+++ fs/nfs/internal.h 2016-01-02 03:19:04.599120855 +1000
@@ -612,9 +612,9 @@
static inline
void nfs_super_set_maxbytes(struct super_block *sb, __u64 maxfilesize)
{
+ if (maxfilesize > MAX_LFS_FILESIZE || maxfilesize == 0)
+ maxfilesize = MAX_LFS_FILESIZE;
sb->s_maxbytes = (loff_t)maxfilesize;
- if (sb->s_maxbytes > MAX_LFS_FILESIZE || sb->s_maxbytes <= 0)
- sb->s_maxbytes = MAX_LFS_FILESIZE;
}
/*
-- AD
On Thu, Mar 17, 2016 at 7:13 AM, Alexey Dvoichenkov <[email protected]> wrote:
> Hello. I've found a small bug in what appears to be the maximum file
> size handling code.
>
> The problem here, as far as I understand, is that casting from an
> unsigned type to a signed type, when the latter cannot represent the
> arithmetic value of the former, is UB. In practice, under the PaX size
> overflow protection, this code crashes when mounting from FreeBSD
> servers that send "all ones" in the size field.
>
> Not sure I'm doing things right with the list and I'm not subscribed, so
> please CC.
>
> The fix should look something like this:
>
> --- fs/nfs/internal.h.orig 2015-11-02 10:05:25.000000000 +1000
> +++ fs/nfs/internal.h 2016-01-02 03:19:04.599120855 +1000
> @@ -612,9 +612,9 @@
> static inline
> void nfs_super_set_maxbytes(struct super_block *sb, __u64 maxfilesize)
> {
> + if (maxfilesize > MAX_LFS_FILESIZE || maxfilesize == 0)
> + maxfilesize = MAX_LFS_FILESIZE;
> sb->s_maxbytes = (loff_t)maxfilesize;
> - if (sb->s_maxbytes > MAX_LFS_FILESIZE || sb->s_maxbytes <= 0)
> - sb->s_maxbytes = MAX_LFS_FILESIZE;
> }
>
Why are we having to change _correct_ code in order to work with a
checking tool?
Trond
On Thu, 17 Mar 2016 09:20:14 -0400
Trond Myklebust <[email protected]> wrote:
> On Thu, Mar 17, 2016 at 7:13 AM, Alexey Dvoichenkov <[email protected]> wrote:
> > Hello. I've found a small bug in what appears to be the maximum file
> > size handling code.
> >
> > The problem here, as far as I understand, is that casting from an
> > unsigned type to a signed type, when the latter cannot represent the
> > arithmetic value of the former, is UB. In practice, under the PaX size
> > overflow protection, this code crashes when mounting from FreeBSD
> > servers that send "all ones" in the size field.
> >
> > Not sure I'm doing things right with the list and I'm not subscribed, so
> > please CC.
> >
> > The fix should look something like this:
> >
> > --- fs/nfs/internal.h.orig 2015-11-02 10:05:25.000000000 +1000
> > +++ fs/nfs/internal.h 2016-01-02 03:19:04.599120855 +1000
> > @@ -612,9 +612,9 @@
> > static inline
> > void nfs_super_set_maxbytes(struct super_block *sb, __u64 maxfilesize)
> > {
> > + if (maxfilesize > MAX_LFS_FILESIZE || maxfilesize == 0)
> > + maxfilesize = MAX_LFS_FILESIZE;
> > sb->s_maxbytes = (loff_t)maxfilesize;
> > - if (sb->s_maxbytes > MAX_LFS_FILESIZE || sb->s_maxbytes <= 0)
> > - sb->s_maxbytes = MAX_LFS_FILESIZE;
> > }
> >
>
> Why are we having to change _correct_ code in order to work with a
> checking tool?
Well, as far as I understand the standard, this cast may result in
undefined behaviour, which in my case it does. The fact that the code
crashes only under PaX, but not when compiled "normally", is a
compiler-dependent detail. So I'm not sure, in what sense is the code
correct?
On the other hand, I understand it may be annoying to fix this sort of
virtually harmless stuff. Then, perhaps, another reason to change it is
that in practice people use GRSEC with NFS and things will break.
-- AD
On Thu, 17 Mar 2016 09:20:14 -0400
Trond Myklebust <[email protected]> wrote:
> Why are we having to change _correct_ code in order to work with a
> checking tool?
Oh, and I guess it could be fixed in GRSEC instead.
-- AD
On 18 Mar 2016 at 8:55, Alexey Dvoichenkov wrote:
> On Thu, 17 Mar 2016 09:20:14 -0400
> Trond Myklebust <[email protected]> wrote:
>
> > > --- fs/nfs/internal.h.orig 2015-11-02 10:05:25.000000000 +1000
> > > +++ fs/nfs/internal.h 2016-01-02 03:19:04.599120855 +1000
> > > @@ -612,9 +612,9 @@
> > > static inline
> > > void nfs_super_set_maxbytes(struct super_block *sb, __u64 maxfilesize)
> > > {
> > > + if (maxfilesize > MAX_LFS_FILESIZE || maxfilesize == 0)
> > > + maxfilesize = MAX_LFS_FILESIZE;
> > > sb->s_maxbytes = (loff_t)maxfilesize;
> > > - if (sb->s_maxbytes > MAX_LFS_FILESIZE || sb->s_maxbytes <= 0)
> > > - sb->s_maxbytes = MAX_LFS_FILESIZE;
> > > }
> > >
> >
> > Why are we having to change _correct_ code in order to work with a
> > checking tool?
first of all, it's correct in that it relies on implementation defined
behaviour and not UB. C99 6.3.1.3 says:
Otherwise, the new type is signed and the value cannot be represented
in it; either the result is implementation-defined or an implementation-
defined signal is raised.
it's still unfortunate of course as the reliance on the above is trivially
avoidable (and would also satisfy the size overflow plugin which otherwise
chooses the other possible behaviour). all that said, i think the bigger
problem with this code pattern is that it's putting the cart before the
horses so to speak, so it'd be better if it was changed for that reason
alone.