Hi Murtaza, On 12/08/2026 08:10, Murtaza wrote: > Hi Jerome, > > Thanks for the suggestion. Yes I think your fix is more robust. Do you want > me to give another patch here?
Yes please. Thanks, -- Jerome > > Best, > Murtaza > > On Thu, Jul 23, 2026 at 8:10 AM Jerome Forissier <[email protected] > <mailto:[email protected]>> wrote: > > Hi Murtaza, > > On 22/07/2026 22:47, Murtaza Munaim wrote: > > nfs_read_reply() reads the NFS READ reply count into a signed int rlen > > via ntohl() and then bounds-checks it before passing it to store_block() > > as an unsigned length: > > > > if (((uchar *)&rpc_pkt.u.reply.data[0] - (uchar *)&rpc_pkt + > rlen) > len) > > return -9999; > > if (store_block(data_ptr, nfs_offset, rlen)) > > return -9999; > > > > The left-hand side of the check is a pointer difference (ptrdiff_t) plus > > the signed rlen, compared against the unsigned int len. A malicious NFS > > server can set the READ count field to a value with the top bit set, so > > that rlen becomes negative. > > > > On ILP32 targets the pointer difference is a 32-bit signed value and the > > comparison against unsigned len wraps the negative sum to a large > > unsigned quantity, so the check triggers and the reply is rejected. On > > LP64 targets the pointer difference is a 64-bit ptrdiff_t and len > > promotes to signed long; the sum stays negative, the check passes, and > > the negative rlen is then converted to a ~2-4 GB size_t inside > > store_block(), where memcpy() copies far past the packet buffer and the > > image load address. The commit that added this check only sanitised the > > length on ILP32. > > > > Reject negative rlen explicitly before the existing check so it is sound > > on all targets. > > > > Fixes: aa207cf3a6d6 ("CVE-2019-14194/CVE-2019-14198: nfs: fix unbounded > memcpy with a failed length check at nfs_read_reply") > > Signed-off-by: Murtaza Munaim <[email protected] > <mailto:[email protected]>> > > --- > > net/nfs-common.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/net/nfs-common.c b/net/nfs-common.c > > index 637fcfd9bb8..111b95459c6 100644 > > --- a/net/nfs-common.c > > +++ b/net/nfs-common.c > > @@ -744,6 +744,17 @@ static int nfs_read_reply(uchar *pkt, unsigned int > len) > > &rpc_pkt.u.reply.data[4 + nfsv3_data_offset]; > > } > > > > + /* > > + * rlen is read from the wire and used below as an unsigned length > > + * for store_block()/memcpy(). The length check that follows > compares > > + * a pointer difference (ptrdiff_t) plus rlen against len; on > LP64 a > > + * negative rlen keeps the left-hand side negative, so the check > > + * passes and rlen wraps to a huge size_t in store_block(). Reject > > + * negative values explicitly so the check is sound on all > targets. > > + */ > > + if (rlen < 0) > > + return -9999; > > + > > if (((uchar *)&rpc_pkt.u.reply.data[0] - (uchar *)&rpc_pkt + > rlen) > len) > > return -9999; > > Is this correct? Shouldn't data_ptr be used instead? How about: > > diff --git a/net/nfs-common.c b/net/nfs-common.c > index 72d8fd823e33..34b276517987 100644 > --- a/net/nfs-common.c > +++ b/net/nfs-common.c > @@ -694,8 +694,9 @@ static int nfs_readlink_reply(uchar *pkt, unsigned > int len) > static int nfs_read_reply(uchar *pkt, unsigned int len) > { > struct rpc_t rpc_pkt; > - int rlen; > + unsigned int rlen; > uchar *data_ptr; > + size_t data_offset; > > memcpy(&rpc_pkt.u.data[0], pkt, sizeof(rpc_pkt.u.reply)); > > @@ -738,7 +739,8 @@ static int nfs_read_reply(uchar *pkt, unsigned int > len) > &rpc_pkt.u.reply.data[4 + nfsv3_data_offset]; > } > > - if (((uchar *)&rpc_pkt.u.reply.data[0] - (uchar *)&rpc_pkt + > rlen) > len) > + data_offset = data_ptr - (uchar *)&rpc_pkt; > + if (data_offset > len || rlen > len - data_offset) > return -9999; > > if (store_block(data_ptr, nfs_offset, rlen)) > > > Thanks, > -- > Jerome >
