Messages in this thread | | | From | Linus Torvalds <> | Date | Sat, 26 Mar 2022 15:15:19 -0700 | Subject | Re: [PATCH 4/4] file: Fix file descriptor leak in copy_fd_bitmaps() |
| |
Sorry, quoting everything below to bring in Eric Biggers because he touched that particular code last.
And Christian Brauner, because he worked on all teh bitmap code with the whole close_range thing.
I think this is all ok because the number of files aren't just byte-aligned, they are long-aligned:
* We make sure that nr remains a multiple of BITS_PER_LONG - otherwise * bitmaps handling below becomes unpleasant, to put it mildly...
but maybe I'm missing something.
The fact that there's a
Found by Syzkaller (https://github.com/google/syzkaller).
thing in that suggested commit message makes me think there _is_ something I'm missing.
Certainly NR_OPEN_DEFAULT, sane_fdtable_size() and max_fds should always be a multiple of BITS_PER_LONG.
So I don't _think_ there is any bug here, although it might be good to
(a) document that "we explicitly do things in BITS_PER_LONG chunks" even more in places
(b) have people double-check my thinking because clearly that syzcaller thing implies I'm full of crap
Eric, Christian?
Can somebody point to the actual syzkaller report?
Linus
On Sat, Mar 26, 2022 at 7:17 AM Alexey Khoroshilov <khoroshilov@ispras.ru> wrote: > > Looks like bfp has a set of macro suitable for such cases: > > #define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK) > #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3) > #define BITS_ROUNDUP_BYTES(bits) \ > (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits)) > > May be it makes sense to move them to a generic header and to use here? > > -- > Alexey Khoroshilov > > > On 26.03.2022 14:40, Fedor Pchelkin wrote: > > If count argument in copy_fd_bitmaps() is not a multiple of > > BITS_PER_BYTE, then one byte is lost and is not used in further > > manipulations with cpy value in memcpy() and memset() > > causing a leak. The leak was introduced with close_range() call > > using CLOSE_RANGE_UNSHARE flag. > > > > The patch suggests implementing an indicator (named add_byte) > > of count being multiple of BITS_PER_BYTE and adding it to the > > cpy value. > > > > Found by Syzkaller (https://github.com/google/syzkaller). > > > > Signed-off-by: Fedor Pchelkin <aissur0002@gmail.com> > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > > --- > > fs/file.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/fs/file.c b/fs/file.c > > index 3ef1479df203..3c64a6423604 100644 > > --- a/fs/file.c > > +++ b/fs/file.c > > @@ -56,10 +56,8 @@ static void copy_fd_bitmaps(struct fdtable *nfdt, struct fdtable *ofdt, > > { > > unsigned int cpy, set; > > unsigned int add_byte = 0; > > - > > if (count % BITS_PER_BYTE != 0) > > add_byte = 1; > > - > > cpy = count / BITS_PER_BYTE + add_byte; > > set = (nfdt->max_fds - count) / BITS_PER_BYTE; > > memcpy(nfdt->open_fds, ofdt->open_fds, cpy); > > >
| |