Re: [PATCH v2 0/3] net: nfs: bound server-supplied lengths in READ and READLINK replies

[Jerome Forissier <jerome.forissier@arm.com>]

Hi Shahriyar,

On 13/08/2026 17:48, Shahriyar Jalayeri wrote:
> A malicious NFS server can return replies whose 32-bit lengths are crafted
> to defeat the client's bounds checks.
>
> nfs_read_reply() keeps the READ length in a signed int. On LP64 a value
> with the top bit set is negative, the bounds check passes, and
> store_block() then hands a ~2 GB length to memcpy(), which reads past the
> 1152-byte reply buffer on the stack and writes past image_load_addr.
>
> nfs_readlink_reply() has the same signed-length flaw. A length of -1 slips
> past the destination bound as pathlen - 1 and drives a memcpy() off
> nfs_path_buff. The bound is also measured from the reply header rather than
> from the symlink data, so a large positive length reads a few bytes past
> the received reply. A server reaches this handler by answering the READ
> with an ISDIR status, which moves the client into the readlink state.
>
> Both handlers are shared by the classic and lwIP NFS clients through
> nfs_pkt_recv().
>
> Patch 1 bounds the READ length by NFS_READ_SIZE. Patch 2 rejects a negative
> readlink length and measures its bound from the symlink data. Patch 3
> enables CONFIG_CMD_NFS in the sandbox config and adds DM regression tests
> that drive nfs_pkt_recv() with crafted replies.


I think the rlen < 0 check in patches 1 and 2 is awkward and not fully robust.
I suggest making rlen a u32 instead.

- For nfs_read_reply() in patch 1:

  u32 rlen;
  size_t data_offset;

  rlen = ntohl(rpc_pkt.u.reply.data[3 + nfsv3_data_offset]);
  if (rlen > NFS_READ_SIZE)
          return -9999;

  data_offset = data_ptr - (uchar *)&rpc_pkt;
  if (data_offset > len || rlen > len - data_offset)
          return -9999;

- For nfs_readlink_reply() in patch 2:

  u32 rlen;
  size_t data_offset;

  rlen = ntohl(rpc_pkt.u.reply.data[1 + nfsv3_data_offset]);
  data_offset = (uchar *)&rpc_pkt.u.reply.data[2 + nfsv3_data_offset] -
                (uchar *)&rpc_pkt;

  if (data_offset > len || rlen > len - data_offset)
           return -NFS_RPC_DROP;

In addition, nfs_readlink_reply() has this check:

  if (pathlen + rlen >= sizeof(nfs_path_buff))

IMO pathlen should be size_t and the test replaced with:

  if (pathlen >= sizeof(nfs_path_buff) ||
      rlen >= sizeof(nfs_path_buff) - pathlen)
          return -NFS_RPC_DROP;

What do you think?

Thanks,
--
Jerome

>
> A reproducer is available on request.
>
> Signed-off-by: Shahriyar Jalayeri <shahriyar@byteray.co.uk>
> ---
> Changes in v2:
> - Add a fix for the same signed-length flaw in nfs_readlink_reply(),
>   rejecting a negative length and measuring the bound from the symlink
>   data.
> - Enable CONFIG_CMD_NFS in sandbox_defconfig so the regression tests are
>   built and run under sandbox; the v1 test was skipped in CI.
> - Add a readlink regression test alongside the read one.
>
> ---
> Shahriyar Jalayeri (3):
>       net: nfs: reject a negative or oversized NFS read length
>       net: nfs: reject a negative or oversized readlink length
>       test: dm: nfs: add regression tests for the NFS reply-length checks
>
>  configs/sandbox_defconfig |  1 +
>  net/nfs-common.c          |  9 ++++-
>  test/dm/Makefile          |  1 +
>  test/dm/nfs.c             | 90 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+), 1 deletion(-)
> ---
> base-commit: baa64b2f892890f00a377eac4a3e685472bb56b5
> change-id: 20260811-nfs-oob-fix-31c433a22c91
>
> Best regards,
> --
> Shahriyar Jalayeri <shahriyar@byteray.co.uk>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.