Re: [PATCH 2/2] net: nfs: reject negative rlen in nfs_read_reply()

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
>