From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Bruce Fields <bfields@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1] NFSD: Fix exposure in nfsd4_decode_bitmap()
Thread-Topic: [PATCH v1] NFSD: Fix exposure in nfsd4_decode_bitmap()
Thread-Index: AQHX2ZR6iDCoondE30yauXAcPh/lE6wDknSAgAEMt4A=
Date:   Mon, 15 Nov 2021 14:00:37 +0000
Message-ID: <75118200-2FC4-41F6-B3FE-7E7DAA46F927@oracle.com>
References: <163692036074.16710.5678362976688977923.stgit@klimt.1015granger.net>
 <41221367b0ac816400bd207e08a3c4e17af1c21e.camel@hammerspace.com>
In-Reply-To: <41221367b0ac816400bd207e08a3c4e17af1c21e.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?/5SKUTK6vo9gZJxmjugigVXrbVWBuznBxZCJ9q+pVVSom30w8XnsiT/UrRkn?=
 =?us-ascii?Q?4A8vvduYbjqY7Jhj1xlrrMkggTNq21E+/gLGWGGvDiiJe48A1zm1Llb3Blf7?=
 =?us-ascii?Q?JESI3PYKbwE0Q5neBmacqVfYvoIRj//3487mEfHdVd2UXDAFUE0MutTsCaNn?=
 =?us-ascii?Q?9YYX8rJCOdI2JcgD0pJDfgoHuEuw+ReXIUVylFmYKcXZitAbPe/8c5Iaho0x?=
 =?us-ascii?Q?Ccm8yhCSdZfufvA/a7Sh4Y1QOYAi1T5rYJxsQa+UuKkBL8mE3B0DlifyMAVq?=
 =?us-ascii?Q?CmfZSzSSAkumqCouGERe11AjLoh31T9PrQjDPFf+Gz0SgNpmeJQztHRW1eal?=
 =?us-ascii?Q?HqLlihoCVlb2v868A+s/Ys6m9OJhH1iCa+nqdafZNpG/2fBDFpQiYJMN8rNF?=
 =?us-ascii?Q?OY2EqoBjO5jkkGczGhBp1sTooQrYapWruP6OoCBMwZfodqoGYvPSMgObvnMx?=
 =?us-ascii?Q?cfc2EYDE/iYj21S2ou8Ie6yzOiopfRaEPQXSDB2ZGmB6QWhfjirBbcQrt+tf?=
 =?us-ascii?Q?cU9DHDsNtkQ/0lIYyprIedBCYzMh7YffQ12j713WppPUDJVF139ju+e1n1QX?=
 =?us-ascii?Q?j76Wqj1Oh9YoxpsHUhPnSI+Y0qKyboHGOu7uVMtrvHEdoDWefMTNLAYxTygW?=
 =?us-ascii?Q?XlOSV7B3mQqi0u46LdYiYtXBoMANaGrVzTy/zp8te3zfvk7YvfDYBZypCZ+b?=
 =?us-ascii?Q?OWjr+eYYilRIQBBU1UuehYVHVw17vJEFdBFcF+7AgkWbn4s7sHmIIJuuM2Iy?=
 =?us-ascii?Q?YXfkHjl0t/G9mB9WeUupI8oVH96VQaRjvbtQcE7E1Bl7lXEl1s8gTzsMkd+r?=
 =?us-ascii?Q?PRqnbIfPe4YyW+mEV4AIso5at27hfReHE4QiKnRoeyJPQ2WkhnawQUumfd9v?=
 =?us-ascii?Q?TzFCHuVcmh+ywpJhsJrFZnos0kTFu7rOhm6z+s7KeiWo6SYfl53Tk8O9gIzI?=
 =?us-ascii?Q?bj3IGtE2BMiKzO+8yByNADXlV/IEebAp0ApbVxc9MMpgO1Fk0378Pj2DDXek?=
 =?us-ascii?Q?g4vGRYZy/q5JDwnimzJwyzTw730tEkxsaxxHhSvJSCMzf/e7wUPzjZElpndx?=
 =?us-ascii?Q?Clxcxr1ObEz1ClDFPRK8EgryEk2tRBSAX/hJwPNCEBnwdwDFIy25bHqPHYFs?=
 =?us-ascii?Q?9VGKkKowG9HXGuTBaVq5lqbmN+v8KGIxYn+EoD2VYkjcOD154G9qz+H6w++7?=
 =?us-ascii?Q?OlgONlMjivdCmt84rba69dUX8Xy9TaETPJHIrGIwFxxSa5+r2asMW0IgLVw5?=
 =?us-ascii?Q?RfndAkwHUPFLPARIpIPCXR4NaNuaj1WsnVQ+yiBjBdlOy1AzOvPz61B0EkO3?=
 =?us-ascii?Q?15OC59xy3vXzdk5y3Bslx84sTdng+bUQ1FWIxsQ9h/9YRDfyvMf5nYB/NLXx?=
 =?us-ascii?Q?4HrWrUOyWekm0KfXfUy7tFtEdtUTrkU6lUWOiZI+fNaLtkIWlR8Il4kN4xEC?=
 =?us-ascii?Q?HkUzoLWFbe2ZbFrsxsTe3nHT6/e9B9wxasuMbEmRs7os9SMzom7BI1vzXHOy?=
 =?us-ascii?Q?+hb25DOh3VsDC0uU/TQ73RXZKW9TfoQUy8mHmuaJa+EDq/Ye1HPfp89DzxPe?=
 =?us-ascii?Q?cjTZvCe8KLlqC6KGmZBpwiitYhe6gdrjOVgLNKtXs94TVUf6foPvmg42M3YC?=
 =?us-ascii?Q?1oAKhjksD87103bTEML/Q0U=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <A2E8FCB8E8BDD94B9BBB632345DA28A2@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1817e383-d2a0-4a3a-a1fc-08d9a8404d7f
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Nov 2021 14:00:37.9984
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2sZOTmBA0lul8PeOpdNg3MouvPWi+CI7lN8uXtbXm8cFawNhfbd6Bd38y4/28wdwovkFhCP2P9/KrpTez54I8Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4623
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10168 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 mlxscore=0
 phishscore=0 bulkscore=0 mlxlogscore=999 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111150075
X-Proofpoint-GUID: KKxig53YBJMM_oei5McIcgr4duuBPWsn
X-Proofpoint-ORIG-GUID: KKxig53YBJMM_oei5McIcgr4duuBPWsn
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Nov 14, 2021, at 4:58 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Sun, 2021-11-14 at 15:16 -0500, Chuck Lever wrote:
>> rtm@csail.mit.edu reports:
>>> nfsd4_decode_bitmap4() will write beyond bmval[bmlen-1] if the RPC
>>> directs it to do so. This can cause nfsd4_decode_state_protect4_a()
>>> to write client-supplied data beyond the end of
>>> nfsd4_exchange_id.spo_must_allow[] when called by
>>> nfsd4_decode_exchange_id().
>>=20
>> Rewrite the loops so nfsd4_decode_bitmap() cannot iterate beyond
>> @bmlen.
>>=20
>> Reported by: rtm@csail.mit.edu
>> Fixes: d1c263a031e8 ("NFSD: Replace READ* macros in
>> nfsd4_decode_fattr()")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |    7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>=20
>> Hi Bruce-
>>=20
>> This version is cleaned up slightly and has been tested as follows:
>>=20
>> - I am not able to crash a patched server using rtm's nfsd_1
>> - No new FAILUREs with NFSv4.1 pynfs tests
>> - No new failures with xfstests on NFSv4.1 or NFSv4.2
>> - No new failures with the git regression suite on NFSv4.1 or NFSv4.2
>> - No reports of NFS4ERR_BADXDR or GARBAGE_ARGS during xfs or git regr
>>=20
>> Hopefully that exercises enough uses of nfsd4_decode_bitmap() to be
>> confident that it is working properly.
>>=20
>> I tested this on top of my XDR tracepoint series, so the line numbers
>> might be a little off from your current tree. However, it should
>> otherwise apply cleanly.
>>=20
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 5ff481e9c85d..479d3452ce6c 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -288,11 +288,8 @@ nfsd4_decode_bitmap4(struct nfsd4_compoundargs
>> *argp, u32 *bmval, u32 bmlen)
>>         p =3D xdr_inline_decode(argp->xdr, count << 2);
>>         if (!p)
>>                 return nfserr_bad_xdr;
>> -       i =3D 0;
>> -       while (i < count)
>> -               bmval[i++] =3D be32_to_cpup(p++);
>> -       while (i < bmlen)
>> -               bmval[i++] =3D 0;
>> +       for (i =3D 0; i < bmlen; i++)
>> +               bmval[i] =3D (i < count) ? be32_to_cpup(p++) : 0;
>> =20
>>         return nfs_ok;
>>  }
>>=20
>=20
> Why not just convert it to use xdr_stream_decode_uint32_array() instead
> of maintaining this separate open coded version?

Pulling the chain a little more, perhaps all decode_bitmap4 call
sites should be converted to use xdr_stream_decode_uint32_array.
We could consider that as a clean up later; the current fix has
been kept surgical so it can be backported.


--
Chuck Lever