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]
Date:   Mon, 25 Jul 2022 22:08:22 +0000
From:   Vanessa Page <Vebpe@...look.com>
To:     Yifei Liu <yifeliu@...stonybrook.edu>
CC:     "ezk@...stonybrook.edu" <ezk@...stonybrook.edu>,
        "madkar@...stonybrook.edu" <madkar@...stonybrook.edu>,
        David Woodhouse <dwmw2@...radead.org>,
        Richard Weinberger <richard@....at>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Kyeong Yoo <kyeong.yoo@...iedtelesis.co.nz>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] jffs2: correct logic when creating a hole in
 jffs2_write_begin

Koop๐Ÿ˜‚๐Ÿ˜‚๐Ÿ˜‘๐Ÿ˜‘๐Ÿ˜‘๐Ÿ˜Ž๐Ÿ˜„๐Ÿ˜„๐Ÿ˜ด๐Ÿ˜ด๐Ÿ‘€๐Ÿ‘€๐Ÿ’๐Ÿ™ˆ๐Ÿ™ˆ๐Ÿ™ˆ๐Ÿ™ˆ๐Ÿ’๐Ÿ‘€๐Ÿ‘€๐Ÿ˜‘๐Ÿ’๐Ÿ˜๐Ÿ˜ณ๐Ÿ˜Ž๐Ÿ˜ด๐Ÿ˜‘๐Ÿ‘€๐Ÿ˜‘๐Ÿ˜„๐Ÿคฃ๐Ÿคฃ๐Ÿ˜€๐Ÿ˜ƒ๐Ÿ˜‘๐Ÿ˜ด๐Ÿ˜„๐Ÿคฅ๐Ÿคซ๐Ÿ˜ฅ๐Ÿ˜ฐ๐Ÿฅต๐Ÿฅต๐Ÿ˜ก๐Ÿ˜ก๐Ÿคฌ๐Ÿคฏ๐Ÿ˜ญ๐Ÿ˜ญ๐Ÿ˜Ÿ๐Ÿ˜Ÿ๐Ÿฅณ๐Ÿคฉ๐Ÿฅธ๐Ÿฅธ๐Ÿฅธ๐Ÿ™๐Ÿ˜ฉ๐Ÿฅบโ˜น๏ธ๐Ÿ˜ฃ๐Ÿ˜”๐Ÿ˜”๐Ÿ˜”๐Ÿ˜–๐Ÿ˜ฃ๐Ÿ˜ข๐Ÿ˜ก๐Ÿฅถ๐Ÿ˜ฐ๐Ÿ˜ฐ๐Ÿ˜ฅ๐Ÿ˜ฅ๐Ÿ˜ถโ€๐ŸŒซ๏ธ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฏ๐Ÿ˜ถโ€๐ŸŒซ๏ธ๐Ÿฅถ๐Ÿ˜ฐ๐Ÿ˜ฐ๐Ÿคซ๐Ÿคซ๐Ÿ˜“๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ
Ooiggvklpoure๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฌ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคฏ๐Ÿคซ๐Ÿ˜“๐Ÿคฌ๐Ÿคฏ๐Ÿคฏ๐Ÿคฌ๐Ÿคฌ๐Ÿ™๐Ÿ˜’๐Ÿ˜๐Ÿ˜’๐Ÿ˜’๐Ÿ˜Ž๐Ÿฅธ๐Ÿคฉ๐Ÿฅณ๐Ÿ˜Ÿ๐Ÿ˜–๐Ÿฅบ๐Ÿ˜ฉ๐Ÿ˜ฉ๐Ÿคฉ๐Ÿฅณ๐Ÿฅณ๐Ÿ˜Ÿ๐Ÿ˜–๐Ÿ˜ฃ๐Ÿ™๐Ÿ˜ฉโ˜น๏ธ๐Ÿ˜Ÿ๐Ÿ˜ซ๐Ÿ˜ค๐Ÿ˜ญ๐Ÿคฌ๐Ÿคฉ๐Ÿคฉ๐Ÿฅธ๐Ÿ˜Ž๐Ÿ™๐Ÿ˜ฉ๐Ÿ˜ก๐Ÿ˜ต๐Ÿ˜ช๐Ÿ˜ง๐Ÿคฅ๐Ÿคฅ๐Ÿ˜ฏ๐Ÿ˜ต๐Ÿคฎ๐Ÿ˜ฌ๐Ÿคฅ๐Ÿ˜“๐Ÿค—๐Ÿค—๐Ÿ˜ถ๐Ÿ˜ฒ๐Ÿคค๐Ÿค๐Ÿค’๐Ÿค•๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿ˜ตโ€๐Ÿ’ซ๐Ÿค‘๐Ÿ˜ผ๐Ÿ˜ฝโ˜ ๏ธ๐Ÿ’€๐Ÿ’€๐Ÿ‘ป๐Ÿ‘ป๐Ÿ‘ป๐Ÿ‘น๐Ÿ‘น๐Ÿ‘บ๐Ÿ‘ฝ๐Ÿ‘ฝ๐Ÿ’€๐Ÿ’€๐Ÿ‘ป๐Ÿ’€๐Ÿ’€๐Ÿ’€โ˜ ๏ธโ˜ ๏ธโ˜ ๏ธโ˜ ๏ธ

Sent from my iPhone

> On Jul 25, 2022, at 1:04 AM, Vanessa Page <Vebpe@...look.com> wrote:
> 
> ๏ปฟ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿฅฐ๐Ÿ˜๐Ÿ‘Œ๐Ÿ˜๐Ÿ‘Œ๐Ÿ˜๐Ÿ˜๐Ÿ˜๐Ÿ˜๐Ÿ˜โ˜บ๏ธโ˜บ๏ธโ˜บ๏ธโ˜บ๏ธโ˜บ๏ธ๐Ÿ’•๐Ÿ’•๐Ÿ˜š๐Ÿ˜š๐Ÿ˜š๐Ÿ˜š๐Ÿฅฐ๐Ÿ˜š๐Ÿ˜๐Ÿ˜๐Ÿ˜๐Ÿ˜š๐Ÿ˜š๐Ÿ˜โ˜บ๏ธโ˜บ๏ธโ˜บ๏ธ๐Ÿ˜๐Ÿ’•๐Ÿ˜š๐Ÿฅฐ๐Ÿฅฐ๐Ÿ˜โ˜บ๏ธโ˜บ๏ธ๐Ÿ˜š๐Ÿฅฐ๐Ÿ˜โ˜บ๏ธ๐Ÿ˜๐Ÿฅฐ๐Ÿ˜โ˜บ๏ธโ˜บ๏ธ๐Ÿ’•๐Ÿฅฐ๐Ÿฅฐ๐Ÿ˜โ˜บ๏ธโ˜บ๏ธ๐Ÿ˜Š
> 
> Sent from my iPhonโœŒ๏ธ
> 
>> On Jul 25, 2022, at 1:01 AM, Yifei Liu <yifeliu@...stonybrook.edu> wrote:
>> 
>> ๏ปฟBug description and fix:
>> 
>> 1. Write data to a file, say all 1s from offset 0 to 16.
>> 
>> 2. Truncate the file to a smaller size, say 8 bytes.
>> 
>> 3. Write new bytes (say 2s) from an offset past the original size of the
>> file, say at offset 20, for 4 bytes.  This is supposed to create a "hole"
>> in the file, meaning that the bytes from offset 8 (where it was truncated
>> above) up to the new write at offset 20, should all be 0s (zeros).
>> 
>> 4. flush all caches using "echo 3 > /proc/sys/vm/drop_caches" (or unmount
>> and remount) the f/s.
>> 
>> 5. Check the content of the file.  It is wrong.  The 1s that used to be
>> between bytes 9 and 16, before the truncation, have REAPPEARED (they should
>> be 0s).
>> 
>> We wrote a script and helper C program to reproduce the bug
>> (reproduce_jffs2_write_begin_issue.sh, write_file.c, and Makefile).  We can
>> make them available to anyone.
>> 
>> The above example is shown when writing a small file within the same first
>> page.  But the bug happens for larger files, as long as steps 1, 2, and 3
>> above all happen within the same page.
>> 
>> The problem was traced to the jffs2_write_begin code, where it goes into an
>> 'if' statement intended to handle writes past the current EOF (i.e., writes
>> that may create a hole).  The code computes a 'pageofs' that is the floor
>> of the write position (pos), aligned to the page size boundary.  In other
>> words, 'pageofs' will never be larger than 'pos'.  The code then sets the
>> internal jffs2_raw_inode->isize to the size of max(current inode size,
>> pageofs) but that is wrong: the new file size should be the 'pos', which is
>> larger than both the current inode size and pageofs.
>> 
>> Similarly, the code incorrectly sets the internal jffs2_raw_inode->dsize to
>> the difference between the pageofs minus current inode size; instead it
>> should be the current pos minus the current inode size.  Finally,
>> inode->i_size was also set incorrectly.
>> 
>> The patch below fixes this bug.  The bug was discovered using a new tool
>> for finding f/s bugs using model checking, called MCFS (Model Checking File
>> Systems).
>> 
>> Signed-off-by: Yifei Liu <yifeliu@...stonybrook.edu>
>> Signed-off-by: Erez Zadok <ezk@...stonybrook.edu>
>> Signed-off-by: Manish Adkar <madkar@...stonybrook.edu>
>> ---
>> fs/jffs2/file.c | 15 +++++++--------
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>> 
>> diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
>> index ba86acbe12d3..0479096b96e4 100644
>> --- a/fs/jffs2/file.c
>> +++ b/fs/jffs2/file.c
>> @@ -137,19 +137,18 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>>   struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
>>   struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
>>   pgoff_t index = pos >> PAGE_SHIFT;
>> -    uint32_t pageofs = index << PAGE_SHIFT;
>>   int ret = 0;
>> 
>>   jffs2_dbg(1, "%s()\n", __func__);
>> 
>> -    if (pageofs > inode->i_size) {
>> -        /* Make new hole frag from old EOF to new page */
>> +    if (pos > inode->i_size) {
>> +        /* Make new hole frag from old EOF to new position */
>>       struct jffs2_raw_inode ri;
>>       struct jffs2_full_dnode *fn;
>>       uint32_t alloc_len;
>> 
>> -        jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
>> -              (unsigned int)inode->i_size, pageofs);
>> +        jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new position\n",
>> +              (unsigned int)inode->i_size, (uint32_t)pos);
>> 
>>       ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
>>                     ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>> @@ -169,10 +168,10 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>>       ri.mode = cpu_to_jemode(inode->i_mode);
>>       ri.uid = cpu_to_je16(i_uid_read(inode));
>>       ri.gid = cpu_to_je16(i_gid_read(inode));
>> -        ri.isize = cpu_to_je32(max((uint32_t)inode->i_size, pageofs));
>> +        ri.isize = cpu_to_je32((uint32_t)pos);
>>       ri.atime = ri.ctime = ri.mtime = cpu_to_je32(JFFS2_NOW());
>>       ri.offset = cpu_to_je32(inode->i_size);
>> -        ri.dsize = cpu_to_je32(pageofs - inode->i_size);
>> +        ri.dsize = cpu_to_je32((uint32_t)pos - inode->i_size);
>>       ri.csize = cpu_to_je32(0);
>>       ri.compr = JFFS2_COMPR_ZERO;
>>       ri.node_crc = cpu_to_je32(crc32(0, &ri, sizeof(ri)-8));
>> @@ -202,7 +201,7 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
>>           goto out_err;
>>       }
>>       jffs2_complete_reservation(c);
>> -        inode->i_size = pageofs;
>> +        inode->i_size = pos;
>>       mutex_unlock(&f->sem);
>>   }
>> 
>> -- 
>> 2.25.1
>> 
>> 
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ