Re: [PATCH] dlm: validate userspace lock resource name length

[Alexander Aring]

Hi,

On Mon, Jun 8, 2026 at 8:16 PM Samuel Moelius
<sam.moelius@xxxxxxxxxxxxxxx> wrote:
>
> The DLM userspace device accepts a flexible resource name after
> `struct dlm_write_request`.  `device_write()` bounded the total write
> size, but did not verify that `i.lock.namelen` was covered by the bytes
> actually supplied by the write.
>
> A short `DLM_USER_LOCK` request can therefore claim a full
> `DLM_RESNAME_MAXLEN` resource name while providing no name bytes.  The
> request path later hashes and copies the claimed name length, reading
> past the `memdup_user_nul()` allocation.
>
> Reject non-conversion lock requests whose claimed resource name length
> exceeds the flexible name payload supplied with the write.  Valid lock
> requests with complete names are unchanged.  Track the payload length
> before compat conversion so 32-bit requests keep using their own request
> header size.
>
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@xxxxxxxxxxxxxxx>

Acked-by: Alexander Aring <aahringo@xxxxxxxxxx>

> ---
>  fs/dlm/user.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> index a8ed4c8fdc5b..205364329ce3 100644
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -511,6 +511,7 @@ static ssize_t device_write(struct file *file, const char __user *buf,
>                             size_t count, loff_t *ppos)
>  {
>         struct dlm_user_proc *proc = file->private_data;
> +       size_t name_payload = 0;
>         struct dlm_write_request *kbuf;
>         int error;
>
> @@ -544,6 +545,7 @@ static ssize_t device_write(struct file *file, const char __user *buf,
>
>                 if (count > sizeof(struct dlm_write_request32))
>                         namelen = count - sizeof(struct dlm_write_request32);
> +               name_payload = namelen;
>
>                 k32buf = (struct dlm_write_request32 *)kbuf;
>
> @@ -560,7 +562,13 @@ static ssize_t device_write(struct file *file, const char __user *buf,
>
>                 compat_input(kbuf, k32buf, namelen);
>                 kfree(k32buf);
> +       } else {
> +               if (count > sizeof(*kbuf))
> +                       name_payload = count - sizeof(*kbuf);
>         }
> +#else
> +       if (count > sizeof(*kbuf))
> +               name_payload = count - sizeof(*kbuf);
>  #endif
>
>         /* do we really need this? can a write happen after a close? */
> @@ -570,6 +578,15 @@ static ssize_t device_write(struct file *file, const char __user *buf,
>                 goto out_free;
>         }
>
> +       if (kbuf->cmd == DLM_USER_LOCK &&
> +           !(kbuf->i.lock.flags & DLM_LKF_CONVERT)) {
> +               if (kbuf->i.lock.namelen > name_payload ||
> +                   kbuf->i.lock.namelen > DLM_RESNAME_MAXLEN) {

there is later a check in "set_lock_args" to do a "if (!(flags &
DLM_LKF_CONVERT) && (namelen > DLM_RESNAME_MAXLEN))" check.
However there is a trace call in between that can access this array
out of bounds area if namelen is above DLM_RESNAME_MAXLEN.

I will add a cleanup patch to my list to check for "sane" parameters
as soon as they are known...

- Alex