decode_bitmap will only decode up to three bitmaps. If the xdr buffer
has more than three bitmaps, return -EIO here instead of bailing out in
a later xdr decode.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
This is related to my "NFSv4: fix getacl ERANGE for some ACL buffer sizes"
patch - I noticed that even though we'll only ever parse 3 bitmaps, we don't
error out correctly if more are sent.
This condition is probably never hit, but if it ever is, it'd be nice to
have the code error out where the problem actually occurred.
fs/nfs/nfs4xdr.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5be2868..3866a69 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3146,6 +3146,9 @@ static int decode_attr_bitmap(struct xdr_stream *xdr, uint32_t *bitmap)
goto out_overflow;
bmlen = be32_to_cpup(p);
+ if (unlikely(bmlen > 3))
+ goto out_overflow;
+
bitmap[0] = bitmap[1] = bitmap[2] = 0;
p = xdr_inline_decode(xdr, (bmlen << 2));
if (unlikely(!p))
--
1.8.3.1 (Apple Git-46)
On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
+AD4- On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
+AD4- +AD4- decode+AF8-bitmap will only decode up to three bitmaps. If the xdr buffer
+AD4- +AD4- has more than three bitmaps, return -EIO here instead of bailing out in
+AD4- +AD4- a later xdr decode.
+AD4- +AD4-
+AD4-
+AD4- No. decode+AF8-bitmap will only +AF8-save+AF8- 3 words in the bitmap+AFsAXQ- argment, but
+AD4- it will decode arbitrary sized bitmaps:
+AD4-
+AD4- p +AD0- xdr+AF8-inline+AF8-decode(xdr, (bmlen +ADwAPA- 2))+ADs-
+AD4-
That said, we should probably check that the server isn't setting those
bitmap words to any non-zero values. That would be a reason to return
EIO.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Fri, 2013-11-15 at 12:07 -0500, Chuck Lever wrote:
+AD4- On Nov 15, 2013, at 12:05 PM, +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4-
+AD4- +AD4- On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
+AD4- +AD4APg- On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
+AD4- +AD4APgA+- decode+AF8-bitmap will only decode up to three bitmaps. If the xdr buffer
+AD4- +AD4APgA+- has more than three bitmaps, return -EIO here instead of bailing out in
+AD4- +AD4APgA+- a later xdr decode.
+AD4- +AD4APgA+-
+AD4- +AD4APg-
+AD4- +AD4APg- No. decode+AF8-bitmap will only +AF8-save+AF8- 3 words in the bitmap+AFsAXQ- argment, but
+AD4- +AD4APg- it will decode arbitrary sized bitmaps:
+AD4- +AD4APg-
+AD4- +AD4APg- p +AD0- xdr+AF8-inline+AF8-decode(xdr, (bmlen +ADwAPA- 2))+ADs-
+AD4- +AD4APg-
+AD4- +AD4-
+AD4- +AD4- That said, we should probably check that the server isn't setting those
+AD4- +AD4- bitmap words to any non-zero values. That would be a reason to return
+AD4- +AD4- EIO.
+AD4-
+AD4- Why wouldn't the client simply warn and ignore the extraneous data?
+AD4-
...because unless the GETATTR is the very last operation, we'd end up
failing to decode things correctly. Anyway, a server that returns
attributes that we haven't requested must clearly be borken. It's
definitely violating the spec.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Nov 15, 2013, at 12:05 PM, "Myklebust, Trond" <[email protected]> wrote:
> On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
>> On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
>>> decode_bitmap will only decode up to three bitmaps. If the xdr buffer
>>> has more than three bitmaps, return -EIO here instead of bailing out in
>>> a later xdr decode.
>>>
>>
>> No. decode_bitmap will only _save_ 3 words in the bitmap[] argment, but
>> it will decode arbitrary sized bitmaps:
>>
>> p = xdr_inline_decode(xdr, (bmlen << 2));
>>
>
> That said, we should probably check that the server isn't setting those
> bitmap words to any non-zero values. That would be a reason to return
> EIO.
Why wouldn't the client simply warn and ignore the extraneous data?
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Nov 15, 2013, at 11:38 AM, Weston Andros Adamson <[email protected]> wrote:
> decode_bitmap will only decode up to three bitmaps. If the xdr buffer
> has more than three bitmaps, return -EIO here instead of bailing out in
> a later xdr decode.
>
> Signed-off-by: Weston Andros Adamson <[email protected]>
> ---
>
> This is related to my "NFSv4: fix getacl ERANGE for some ACL buffer sizes"
> patch - I noticed that even though we'll only ever parse 3 bitmaps, we don't
> error out correctly if more are sent.
>
> This condition is probably never hit, but if it ever is, it'd be nice to
> have the code error out where the problem actually occurred.
>
> fs/nfs/nfs4xdr.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5be2868..3866a69 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -3146,6 +3146,9 @@ static int decode_attr_bitmap(struct xdr_stream *xdr, uint32_t *bitmap)
> goto out_overflow;
> bmlen = be32_to_cpup(p);
>
> + if (unlikely(bmlen > 3))
> + goto out_overflow;
> +
Nit: Using a naked integer should be avoid, if we can. Is there somewhere in the code that documents the "we only handle up t 3 bitmaps" constraint?
> bitmap[0] = bitmap[1] = bitmap[2] = 0;
> p = xdr_inline_decode(xdr, (bmlen << 2));
> if (unlikely(!p))
> --
> 1.8.3.1 (Apple Git-46)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Nov 15, 2013, at 12:10 PM, Myklebust, Trond <[email protected]> wrote:
> On Fri, 2013-11-15 at 12:07 -0500, Chuck Lever wrote:
>> On Nov 15, 2013, at 12:05 PM, "Myklebust, Trond" <[email protected]> wrote:
>>
>>> On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
>>>> On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
>>>>> decode_bitmap will only decode up to three bitmaps. If the xdr buffer
>>>>> has more than three bitmaps, return -EIO here instead of bailing out in
>>>>> a later xdr decode.
>>>>>
>>>>
>>>> No. decode_bitmap will only _save_ 3 words in the bitmap[] argment, but
>>>> it will decode arbitrary sized bitmaps:
>>>>
>>>> p = xdr_inline_decode(xdr, (bmlen << 2));
>>>>
Oh, yeah.
>>>
>>> That said, we should probably check that the server isn't setting those
>>> bitmap words to any non-zero values. That would be a reason to return
>>> EIO.
Ok, I?ll rework this.
-dros
>>
>> Why wouldn't the client simply warn and ignore the extraneous data?
>>
>
> ...because unless the GETATTR is the very last operation, we'd end up
> failing to decode things correctly. Anyway, a server that returns
> attributes that we haven't requested must clearly be borken. It's
> definitely violating the spec.
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
On Fri, 2013-11-15 at 12:23 -0500, Chuck Lever wrote:
+AD4- On Nov 15, 2013, at 12:10 PM, +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4-
+AD4- +AD4- On Fri, 2013-11-15 at 12:07 -0500, Chuck Lever wrote:
+AD4- +AD4APg- On Nov 15, 2013, at 12:05 PM, +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4- +AD4APg-
+AD4- +AD4APgA+- On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
+AD4- +AD4APgA+AD4- On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
+AD4- +AD4APgA+AD4APg- decode+AF8-bitmap will only decode up to three bitmaps. If the xdr buffer
+AD4- +AD4APgA+AD4APg- has more than three bitmaps, return -EIO here instead of bailing out in
+AD4- +AD4APgA+AD4APg- a later xdr decode.
+AD4- +AD4APgA+AD4APg-
+AD4- +AD4APgA+AD4-
+AD4- +AD4APgA+AD4- No. decode+AF8-bitmap will only +AF8-save+AF8- 3 words in the bitmap+AFsAXQ- argment, but
+AD4- +AD4APgA+AD4- it will decode arbitrary sized bitmaps:
+AD4- +AD4APgA+AD4-
+AD4- +AD4APgA+AD4- p +AD0- xdr+AF8-inline+AF8-decode(xdr, (bmlen +ADwAPA- 2))+ADs-
+AD4- +AD4APgA+AD4-
+AD4- +AD4APgA+-
+AD4- +AD4APgA+- That said, we should probably check that the server isn't setting those
+AD4- +AD4APgA+- bitmap words to any non-zero values. That would be a reason to return
+AD4- +AD4APgA+- EIO.
+AD4- +AD4APg-
+AD4- +AD4APg- Why wouldn't the client simply warn and ignore the extraneous data?
+AD4- +AD4APg-
+AD4- +AD4-
+AD4- +AD4- ...because unless the GETATTR is the very last operation, we'd end up
+AD4- +AD4- failing to decode things correctly.
+AD4-
+AD4- Surely that's only if the returned bitmap length doesn't match the number of bitmap words returned. The server can return a properly encoded result without overwriting the next operation in the compound, can't it?
How do we know? You're already talking about a broken server.
+AD4- +AD4- Anyway, a server that returns
+AD4- +AD4- attributes that we haven't requested must clearly be borken. It's
+AD4- +AD4- definitely violating the spec.
+AD4-
+AD4- Definitely, but +ACI-be lenient in what you accept.+ACI-
+AD4-
+AD4- The reason I bring this up is that we had exactly this problem with NFSv4.2, where the third bitmap word was added.
We've never had servers returning attributes that are not requested
AFAIK. In the current code, they are free to add in as many zero fillers
in the bitmap as they want, and that's exactly what we should be
accepting.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
+AD4 decode+AF8-bitmap will only decode up to three bitmaps. If the xdr buffer
+AD4 has more than three bitmaps, return -EIO here instead of bailing out in
+AD4 a later xdr decode.
+AD4
No. decode+AF8-bitmap will only +AF8-save+AF8 3 words in the bitmap+AFsAXQ argment, but
it will decode arbitrary sized bitmaps:
p +AD0 xdr+AF8-inline+AF8-decode(xdr, (bmlen +ADwAPA 2))+ADs
Cheers
Trond
On Nov 15, 2013, at 12:10 PM, "Myklebust, Trond" <[email protected]> wrote:
> On Fri, 2013-11-15 at 12:07 -0500, Chuck Lever wrote:
>> On Nov 15, 2013, at 12:05 PM, "Myklebust, Trond" <[email protected]> wrote:
>>
>>> On Fri, 2013-11-15 at 12:00 -0500, Trond Myklebust wrote:
>>>> On Fri, 2013-11-15 at 11:38 -0500, Weston Andros Adamson wrote:
>>>>> decode_bitmap will only decode up to three bitmaps. If the xdr buffer
>>>>> has more than three bitmaps, return -EIO here instead of bailing out in
>>>>> a later xdr decode.
>>>>>
>>>>
>>>> No. decode_bitmap will only _save_ 3 words in the bitmap[] argment, but
>>>> it will decode arbitrary sized bitmaps:
>>>>
>>>> p = xdr_inline_decode(xdr, (bmlen << 2));
>>>>
>>>
>>> That said, we should probably check that the server isn't setting those
>>> bitmap words to any non-zero values. That would be a reason to return
>>> EIO.
>>
>> Why wouldn't the client simply warn and ignore the extraneous data?
>>
>
> ...because unless the GETATTR is the very last operation, we'd end up
> failing to decode things correctly.
Surely that's only if the returned bitmap length doesn't match the number of bitmap words returned. The server can return a properly encoded result without overwriting the next operation in the compound, can't it?
> Anyway, a server that returns
> attributes that we haven't requested must clearly be borken. It's
> definitely violating the spec.
Definitely, but "be lenient in what you accept."
The reason I bring this up is that we had exactly this problem with NFSv4.2, where the third bitmap word was added.
Anyway, just an observation.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com