linux-kernel - Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878qpi5wz4.fsf@prevas.dk>
Date: Thu, 06 Mar 2025 10:28:47 +0100
From: Rasmus Villemoes <ravi@...vas.dk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Oleg Nesterov <oleg@...hat.com>,  Mateusz Guzik <mjguzik@...il.com>,  K
 Prateek Nayak <kprateek.nayak@....com>,  "Sapkal, Swapnil"
 <swapnil.sapkal@....com>,  Manfred Spraul <manfred@...orfullife.com>,
  Christian Brauner <brauner@...nel.org>,  David Howells
 <dhowells@...hat.com>,  WangYuli <wangyuli@...ontech.com>,
  linux-fsdevel@...r.kernel.org,  linux-kernel@...r.kernel.org,  "Shenoy,
 Gautham Ranjal" <gautham.shenoy@....com>,  Neeraj.Upadhyay@....com,
  Ananth.narayan@....com
Subject: Re: [PATCH] pipe_read: don't wake up the writer if the pipe is
 still full

On Wed, Mar 05 2025, Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> Are there other cases like this? I don't know. I've been looking
> around a bit, but those were the only ones I found immediately when I
> started thinking about the whole wrap-around issue.

Without too much brainpower spent analyzing each case (i.e., some of
these might actually be ok), I found these:

fs/fuse/dev.c
fuse_dev_splice_write()

  unsigned int head, tail, mask, count;

  pipe_lock(pipe);

  head = pipe->head;
  tail = pipe->tail;
  mask = pipe->ring_size - 1;
  count = head - tail;

Open-coded pipe_occupancy(), so would be fixed by using that with your
fixup.

A bit later in same function there's the same FIONREAD pattern:

  for (idx = tail; idx != head && rem < len; idx++)
          rem += pipe->bufs[idx & mask].len;

fs/pipe.c

We have pipe_update_tail() getting and returning an "unsigned int",
and letting the compiler truncate the result written to pipe->tail:

      pipe->tail = ++tail;
      return tail;

pipe_update_tail() only has one caller, but a rather important one,
pipe_read(), which uses the return value from pipe_update_tail as-is

                 tail = pipe_update_tail(pipe, buf, tail);
         }
         total_len -= chars;
         if (!total_len)
                 break;  /* common path: read succeeded */
         if (!pipe_empty(head, tail))    /* More to do? */
                 continue;

and pipe_empty() takes two "unsigned ints" and is just head==tail -- so if
tail was incremented to 65536 while head is 0 that would break. Probably
pipe_empty() should either take pipe_index_t arguments or cast to that
internally, just as pipe_occupancy. Or, as pipe_full(), being spelled in
terms of pipe_occupancy()==0.

With that fixed, maybe one could spell the FIONREAD-like patterns using
pipe_empty(), i.e. using pipe_empty() to ask "have this tail index now
caught up to this head index". So "idx != head" above would become
"!pipe_empty(idx, head)".

Rasmus