| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <97c86b90-76b2-483a-a649-c115ffe22213@embeddedor.com>
Date: Mon, 16 Jun 2025 16:22:32 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Chuck Lever <chuck.lever@...cle.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Christoph Hellwig <hch@...radead.org>, Jeff Layton <jlayton@...nel.org>,
NeilBrown <neil@...wn.name>, Olga Kornievskaia <okorniev@...hat.com>,
Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2][next] NFSD: Avoid multiple
-Wflex-array-member-not-at-end warnings
On 16/06/25 14:30, Chuck Lever wrote:
> On 6/11/25 6:58 PM, Gustavo A. R. Silva wrote:
>> Update `struct knfsd_fh` to use indices into the `fh_raw` array,
>> allowing removal of the flexible-array member `fh_fsid[]`. Refactor
>> related code accordingly.
>>
>> With this changes, fix many instances of the following type of
>> warnings:
>>
>> fs/nfsd/nfsfh.h:79:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> fs/nfsd/state.h:763:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> fs/nfsd/state.h:669:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> fs/nfsd/state.h:549:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> fs/nfsd/xdr4.h:705:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>> fs/nfsd/xdr4.h:678:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>>
>> Co-developed-by: Christoph Hellwig <hch@...radead.org>
>> Signed-off-by: Christoph Hellwig <hch@...radead.org>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>> ---
>> Changes in v2:
>> - Use indices into `fh_raw`. (Christoph)
>> - Remove union and flexible-array member `fh_fsid`. (Christoph)
>>
>> v1:
>> - Link: https://lore.kernel.org/linux-hardening/aBp37ZXBJM09yAXp@kspp/
>
> I prefer Neil's solution to address just the "flexible array member"
> warnings.
>
> The intent in struct knfsd_fh was that fh_fsid was to fill up the rest
> of that arm of the union; it is not flexible in size.
>
> Would it be enough to replace:
>
> u32 fh_fsid[]; /* flexible-array member */
>
> with:
>
> u32 fh_fsid[NFS4_FHSIZE / 4 - 1];
Yep, this works --I'll send this as v3, then.
>
> or:
>
> u32 fh_fsid[0];
Zero-length arrays are deprecated under these scenarios because they lie
to the compiler, and we don't want that.
>
> ?
>
>
> I agree that packing the u8 fields in a struct/union combination and
> then putting the octets directly onto the wire, as is currently done, is
> asking for trouble. However, replacing the union, as Christoph
> suggested, is a clean-up that ought to be done over time in multiple
> patches so that it is done mechanically (perhaps via cocchinelle) and
> can be bisected over if needed.
>
> I might even want to see helper functions to access those fields, rather
> than poking them by symbolic offset into an array. Using accessor
> functions is definitely appropriate for the nfs4layouts.c consumer of
> knfsd_fh.
>
>
>> fs/nfsd/export.c | 2 +-
>> fs/nfsd/export.h | 2 +-
>> fs/nfsd/nfs4layouts.c | 10 ++++----
>> fs/nfsd/nfsfh.c | 58 +++++++++++++++++++++++--------------------
>> fs/nfsd/nfsfh.h | 30 +++++++++++-----------
>> 5 files changed, 52 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
>> index 88ae410b4113..654d54a7148f 100644
>> --- a/fs/nfsd/export.c
>> +++ b/fs/nfsd/export.c
>> @@ -1231,7 +1231,7 @@ rqst_exp_get_by_name(struct svc_rqst *rqstp, struct path *path)
>> struct svc_export *
>> rqst_exp_find(struct cache_req *reqp, struct net *net,
>> struct auth_domain *cl, struct auth_domain *gsscl,
>> - int fsid_type, u32 *fsidv)
>> + int fsid_type, void *fsidv)
>> {
>> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>> struct svc_export *gssexp, *exp = ERR_PTR(-ENOENT);
>> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
>> index 4d92b99c1ffd..9b2719d8a3e2 100644
>> --- a/fs/nfsd/export.h
>> +++ b/fs/nfsd/export.h
>> @@ -131,6 +131,6 @@ static inline struct svc_export *exp_get(struct svc_export *exp)
>> }
>> struct svc_export *rqst_exp_find(struct cache_req *reqp, struct net *net,
>> struct auth_domain *cl, struct auth_domain *gsscl,
>> - int fsid_type, u32 *fsidv);
>> + int fsid_type, void *fsidv);
>>
>> #endif /* NFSD_EXPORT_H */
>> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
>> index 290271ac4245..6dcd2c9ec15b 100644
>> --- a/fs/nfsd/nfs4layouts.c
>> +++ b/fs/nfsd/nfs4layouts.c
>> @@ -56,7 +56,7 @@ static void
>> nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>> {
>> const struct knfsd_fh *fh = &fhp->fh_handle;
>> - size_t fsid_len = key_len(fh->fh_fsid_type);
>> + size_t fsid_len = key_len(fh->fh_raw[FH_FSID_TYPE]);
>> struct nfsd4_deviceid_map *map, *old;
>> int i;
>>
>> @@ -64,8 +64,8 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>> if (!map)
>> return;
>>
>> - map->fsid_type = fh->fh_fsid_type;
>> - memcpy(&map->fsid, fh->fh_fsid, fsid_len);
>> + map->fsid_type = fh->fh_raw[FH_FSID_TYPE];
>> + memcpy(&map->fsid, fh->fh_raw + FH_FSID, fsid_len);
>>
>> spin_lock(&nfsd_devid_lock);
>> if (fhp->fh_export->ex_devid_map)
>> @@ -73,9 +73,9 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>>
>> for (i = 0; i < DEVID_HASH_SIZE; i++) {
>> list_for_each_entry(old, &nfsd_devid_hash[i], hash) {
>> - if (old->fsid_type != fh->fh_fsid_type)
>> + if (old->fsid_type != map->fsid_type)
>> continue;
>> - if (memcmp(old->fsid, fh->fh_fsid,
>> + if (memcmp(old->fsid, map->fsid,
>> key_len(old->fsid_type)))
>> continue;
>>
>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>> index aef474f1b84b..01ff91a28fb6 100644
>> --- a/fs/nfsd/nfsfh.c
>> +++ b/fs/nfsd/nfsfh.c
>> @@ -161,37 +161,40 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
>> if (fh->fh_size == 0)
>> return nfserr_nofilehandle;
>>
>> - if (fh->fh_version != 1)
>> + if (fh->fh_raw[FH_VERSION] != 1)
>> return error;
>>
>> if (--data_left < 0)
>> return error;
>> - if (fh->fh_auth_type != 0)
>> + if (fh->fh_raw[FH_AUTH_TYPE] != 0)
>> return error;
>> - len = key_len(fh->fh_fsid_type) / 4;
>> + len = key_len(fh->fh_raw[FH_FSID_TYPE]) / 4;
>> if (len == 0)
>> return error;
>> - if (fh->fh_fsid_type == FSID_MAJOR_MINOR) {
>> + if (fh->fh_raw[FH_FSID_TYPE] == FSID_MAJOR_MINOR) {
>> /* deprecated, convert to type 3 */
>> + u32 *fsidv = (u32 *)(fh->fh_raw + FH_FSID);
>> +
>> len = key_len(FSID_ENCODE_DEV)/4;
>> - fh->fh_fsid_type = FSID_ENCODE_DEV;
>> + fh->fh_raw[FH_FSID_TYPE] = FSID_ENCODE_DEV;
>> /*
>> * struct knfsd_fh uses host-endian fields, which are
>> * sometimes used to hold net-endian values. This
>> * confuses sparse, so we must use __force here to
>> * keep it from complaining.
>> */
>> - fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fh->fh_fsid[0]),
>> - ntohl((__force __be32)fh->fh_fsid[1])));
>> - fh->fh_fsid[1] = fh->fh_fsid[2];
>> + fsidv[0] = new_encode_dev(MKDEV(
>> + ntohl((__force __be32)fsidv[0]),
>> + ntohl((__force __be32)fsidv[1])));
>> + fsidv[1] = fsidv[2];
>> }
>> data_left -= len;
>> if (data_left < 0)
>> return error;
>> exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL,
>> net, client, gssclient,
>> - fh->fh_fsid_type, fh->fh_fsid);
>> - fid = (struct fid *)(fh->fh_fsid + len);
>> + fh->fh_raw[FH_FSID_TYPE], fh->fh_raw + FH_FSID);
>> + fid = (struct fid *)(fh->fh_raw + FH_FSID + len);
>>
>> error = nfserr_stale;
>> if (IS_ERR(exp)) {
>> @@ -233,7 +236,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
>> */
>> error = nfserr_badhandle;
>>
>> - fileid_type = fh->fh_fileid_type;
>> + fileid_type = fh->fh_raw[FH_FILEID_TYPE];
>>
>> if (fileid_type == FILEID_ROOT)
>> dentry = dget(exp->ex_path.dentry);
>> @@ -463,18 +466,19 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
>> {
>> if (dentry != exp->ex_path.dentry) {
>> struct fid *fid = (struct fid *)
>> - (fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
>> + (fhp->fh_handle.fh_raw + FH_FSID +
>> + fhp->fh_handle.fh_size - 1);
>> int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
>> int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
>> EXPORT_FH_CONNECTABLE;
>> int fileid_type =
>> exportfs_encode_fh(dentry, fid, &maxsize, fh_flags);
>>
>> - fhp->fh_handle.fh_fileid_type =
>> + fhp->fh_handle.fh_raw[FH_FILEID_TYPE] =
>> fileid_type > 0 ? fileid_type : FILEID_INVALID;
>> fhp->fh_handle.fh_size += maxsize * 4;
>> } else {
>> - fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
>> + fhp->fh_handle.fh_raw[FH_FILEID_TYPE] = FILEID_ROOT;
>> }
>> }
>>
>> @@ -520,8 +524,8 @@ static void set_version_and_fsid_type(struct svc_fh *fhp, struct svc_export *exp
>> retry:
>> version = 1;
>> if (ref_fh && ref_fh->fh_export == exp) {
>> - version = ref_fh->fh_handle.fh_version;
>> - fsid_type = ref_fh->fh_handle.fh_fsid_type;
>> + version = ref_fh->fh_handle.fh_raw[FH_VERSION];
>> + fsid_type = ref_fh->fh_handle.fh_raw[FH_FSID_TYPE];
>>
>> ref_fh = NULL;
>>
>> @@ -562,9 +566,9 @@ static void set_version_and_fsid_type(struct svc_fh *fhp, struct svc_export *exp
>> fsid_type = FSID_ENCODE_DEV;
>> else
>> fsid_type = FSID_DEV;
>> - fhp->fh_handle.fh_version = version;
>> + fhp->fh_handle.fh_raw[FH_VERSION] = version;
>> if (version)
>> - fhp->fh_handle.fh_fsid_type = fsid_type;
>> + fhp->fh_handle.fh_raw[FH_FSID_TYPE] = fsid_type;
>> }
>>
>> __be32
>> @@ -610,18 +614,18 @@ fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
>> fhp->fh_export = exp_get(exp);
>>
>> fhp->fh_handle.fh_size =
>> - key_len(fhp->fh_handle.fh_fsid_type) + 4;
>> - fhp->fh_handle.fh_auth_type = 0;
>> + key_len(fhp->fh_handle.fh_raw[FH_FSID_TYPE]) + 4;
>> + fhp->fh_handle.fh_raw[FH_AUTH_TYPE] = 0;
>>
>> - mk_fsid(fhp->fh_handle.fh_fsid_type,
>> - fhp->fh_handle.fh_fsid,
>> + mk_fsid(fhp->fh_handle.fh_raw[FH_FSID_TYPE],
>> + fhp->fh_handle.fh_raw + FH_FSID,
>> ex_dev,
>> d_inode(exp->ex_path.dentry)->i_ino,
>> exp->ex_fsid, exp->ex_uuid);
>>
>> if (inode)
>> _fh_update(fhp, exp, dentry);
>> - if (fhp->fh_handle.fh_fileid_type == FILEID_INVALID) {
>> + if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] == FILEID_INVALID) {
>> fh_put(fhp);
>> return nfserr_stale;
>> }
>> @@ -644,11 +648,11 @@ fh_update(struct svc_fh *fhp)
>> dentry = fhp->fh_dentry;
>> if (d_really_is_negative(dentry))
>> goto out_negative;
>> - if (fhp->fh_handle.fh_fileid_type != FILEID_ROOT)
>> + if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] != FILEID_ROOT)
>> return 0;
>>
>> _fh_update(fhp, fhp->fh_export, dentry);
>> - if (fhp->fh_handle.fh_fileid_type == FILEID_INVALID)
>> + if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] == FILEID_INVALID)
>> return nfserr_stale;
>> return 0;
>> out_bad:
>> @@ -776,9 +780,9 @@ char * SVCFH_fmt(struct svc_fh *fhp)
>>
>> enum fsid_source fsid_source(const struct svc_fh *fhp)
>> {
>> - if (fhp->fh_handle.fh_version != 1)
>> + if (fhp->fh_handle.fh_raw[FH_VERSION] != 1)
>> return FSIDSOURCE_DEV;
>> - switch(fhp->fh_handle.fh_fsid_type) {
>> + switch (fhp->fh_handle.fh_raw[FH_FSID_TYPE]) {
>> case FSID_DEV:
>> case FSID_ENCODE_DEV:
>> case FSID_MAJOR_MINOR:
>> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
>> index 5103c2f4d225..26975ede465a 100644
>> --- a/fs/nfsd/nfsfh.h
>> +++ b/fs/nfsd/nfsfh.h
>> @@ -43,22 +43,17 @@
>> * filesystems must not use the values '0' or '0xff'. 'See enum fid_type'
>> * in include/linux/exportfs.h for currently registered values.
>> */
>> -
>> struct knfsd_fh {
>> unsigned int fh_size; /*
>> * Points to the current size while
>> * building a new file handle.
>> */
>> - union {
>> - char fh_raw[NFS4_FHSIZE];
>> - struct {
>> - u8 fh_version; /* == 1 */
>> - u8 fh_auth_type; /* deprecated */
>> - u8 fh_fsid_type;
>> - u8 fh_fileid_type;
>> - u32 fh_fsid[]; /* flexible-array member */
>> - };
>> - };
>> + char fh_raw[NFS4_FHSIZE];
>> +#define FH_VERSION 0
>> +#define FH_AUTH_TYPE 1
>> +#define FH_FSID_TYPE 2
>> +#define FH_FILEID_TYPE 3
>> +#define FH_FSID 4
>> };
>>
>> static inline __u32 ino_t_to_u32(ino_t ino)
>> @@ -141,10 +136,12 @@ extern enum fsid_source fsid_source(const struct svc_fh *fhp);
>> * sparse from complaining. Since these values are opaque to the
>> * client, that shouldn't be a problem.
>> */
>> -static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
>> - u32 fsid, unsigned char *uuid)
>> +static inline void mk_fsid(int vers, void *fsid, dev_t dev, ino_t ino,
>> + u32 fsid_num, unsigned char *uuid)
>> {
>> + u32 *fsidv = fsid;
>> u32 *up;
>> +
>> switch(vers) {
>> case FSID_DEV:
>> fsidv[0] = (__force __u32)htonl((MAJOR(dev)<<16) |
>> @@ -152,7 +149,7 @@ static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
>> fsidv[1] = ino_t_to_u32(ino);
>> break;
>> case FSID_NUM:
>> - fsidv[0] = fsid;
>> + fsidv[0] = fsid_num;
>> break;
>> case FSID_MAJOR_MINOR:
>> fsidv[0] = (__force __u32)htonl(MAJOR(dev));
>> @@ -260,9 +257,10 @@ static inline bool fh_match(const struct knfsd_fh *fh1,
>> static inline bool fh_fsid_match(const struct knfsd_fh *fh1,
>> const struct knfsd_fh *fh2)
>> {
>> - if (fh1->fh_fsid_type != fh2->fh_fsid_type)
>> + if (fh1->fh_raw[FH_FSID_TYPE] != fh2->fh_raw[FH_FSID_TYPE])
>> return false;
>> - if (memcmp(fh1->fh_fsid, fh2->fh_fsid, key_len(fh1->fh_fsid_type)) != 0)
>> + if (memcmp(fh1->fh_raw + FH_FSID, fh2->fh_raw + FH_FSID,
>> + key_len(fh1->fh_raw[FH_FSID_TYPE])) != 0)
>> return false;
>> return true;
>> }
>
>
-Gustavo
Powered by blists - more mailing lists