| Message ID | 20180108053542.6472-7-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > From: Eric Biggers <ebiggers@google.com> > > round_pipe_size() calculates the number of pages the requested size > corresponds to, then rounds the page count up to the next power of 2. > > However, it also rounds everything < PAGE_SIZE up to PAGE_SIZE. > Therefore, there's no need to actually translate the size into a page > count; we just need to round the size up to the next power of 2. > > We do need to verify that bit 31 isn't set, since on 32-bit systems > roundup_pow_of_two() would be undefined in that case. But that can just > be combined with the UINT_MAX check which we need anyway now. > > Finally, also remove the check for '!nr_pages' in pipe_set_size(), since > round_pipe_size() always returns either 0 or a multiple of PAGE_SIZE. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/pipe.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/fs/pipe.c b/fs/pipe.c > index f1ee1e599495..774cafd947dc 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -1022,20 +1022,14 @@ const struct file_operations pipefifo_fops = { > */ > unsigned int round_pipe_size(unsigned long size) > { > - unsigned long nr_pages; > - > - if (size > UINT_MAX) > + if (size > (1U << 31)) > return 0; > > /* Minimum pipe size, as required by POSIX */ > if (size < PAGE_SIZE) > - size = PAGE_SIZE; > - > - nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; > - if (nr_pages == 0) > - return 0; > + return PAGE_SIZE; > > - return roundup_pow_of_two(nr_pages) << PAGE_SHIFT; > + return roundup_pow_of_two(size); > } > > /* Above looks good. > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) > return -EINVAL; > nr_pages = size >> PAGE_SHIFT; > > - if (!nr_pages) > - return -EINVAL; > - I would just leave this hunk anyway: it's defensive for any future changes. Maybe add a comment describing why it's currently redundant? -Kees > /* > * If trying to increase the pipe capacity, check that an > * unprivileged user is not trying to exceed various limits > -- > 2.15.1 >
On Tue, Jan 09, 2018 at 02:27:10PM -0800, Kees Cook wrote: > > > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) > > return -EINVAL; > > nr_pages = size >> PAGE_SHIFT; > > > > - if (!nr_pages) > > - return -EINVAL; > > - > > I would just leave this hunk anyway: it's defensive for any future > changes. Maybe add a comment describing why it's currently redundant? > I don't know; I find it really confusing to have two slightly different checks for the same thing, as it implies that they actually need to be there for a reason. How about just checking nr_pages? size = round_pipe_size(arg); nr_pages = size >> PAGE_SHIFT; if (nr_pages == 0) return -EINVAL; Eric
On Tue, Jan 9, 2018 at 6:52 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > On Tue, Jan 09, 2018 at 02:27:10PM -0800, Kees Cook wrote: >> >> > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) >> > return -EINVAL; >> > nr_pages = size >> PAGE_SHIFT; >> > >> > - if (!nr_pages) >> > - return -EINVAL; >> > - >> >> I would just leave this hunk anyway: it's defensive for any future >> changes. Maybe add a comment describing why it's currently redundant? >> > > I don't know; I find it really confusing to have two slightly different checks > for the same thing, as it implies that they actually need to be there for a > reason. How about just checking nr_pages? > > size = round_pipe_size(arg); > nr_pages = size >> PAGE_SHIFT; > > if (nr_pages == 0) > return -EINVAL; Oh yeah! I like that. -Kees
diff --git a/fs/pipe.c b/fs/pipe.c index f1ee1e599495..774cafd947dc 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -1022,20 +1022,14 @@ const struct file_operations pipefifo_fops = { */ unsigned int round_pipe_size(unsigned long size) { - unsigned long nr_pages; - - if (size > UINT_MAX) + if (size > (1U << 31)) return 0; /* Minimum pipe size, as required by POSIX */ if (size < PAGE_SIZE) - size = PAGE_SIZE; - - nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; - if (nr_pages == 0) - return 0; + return PAGE_SIZE; - return roundup_pow_of_two(nr_pages) << PAGE_SHIFT; + return roundup_pow_of_two(size); } /* @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) return -EINVAL; nr_pages = size >> PAGE_SHIFT; - if (!nr_pages) - return -EINVAL; - /* * If trying to increase the pipe capacity, check that an * unprivileged user is not trying to exceed various limits