Question about commit "ext4: always initialize the crc32c checksum driver"

Question about commit "ext4: always initialize the crc32c checksum driver"

[zhangyi (F)]

Hi Ted,

I am checking a CVE patch a45403b515 "ext4: always initialize the crc32c checksum driver"[1]
in CVE-2018-1094[2] recently, and have a question about the commit log in this patch.

The patch commit log said:

> The extended attribute code now uses the crc32c checksum for hashing
> purposes, so we should just always always initialize it.  We also want
> to prevent NULL pointer dereferences if one of the metadata checksum
> features is enabled after the file sytsem is originally mounted.

This first fix is clear. But I don't understand the second fix. IIUC, the kernel does not call
ext4_set_feature_metadata_csum() to enable metadata checksum, and this feature can only be enabled
by mkfs,turn2fs or change the image directly. So this feature bit will never change once the
file system is mounted, the second case could never happen ?

BTW, does this patch need on the old kernel before dec214d00e "ext4: xattr inode deduplication" ?

------
[1]. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a45403b51582a
[2]. https://nvd.nist.gov/vuln/detail/CVE-2018-1094

Thanks,
Yi.

Re: Question about commit "ext4: always initialize the crc32c checksum driver"

[Theodore Y. Ts'o]

On Thu, Dec 13, 2018 at 03:56:04PM +0800, zhangyi (F) wrote:
> I am checking a CVE patch a45403b515 "ext4: always initialize the crc32c checksum driver"[1]
> in CVE-2018-1094[2] recently, and have a question about the commit log in this patch.
> 
> The patch commit log said:
> 
> > The extended attribute code now uses the crc32c checksum for hashing
> > purposes, so we should just always always initialize it.  We also want
> > to prevent NULL pointer dereferences if one of the metadata checksum
> > features is enabled after the file sytsem is originally mounted.
> 
> This first fix is clear. But I don't understand the second fix. IIUC, the kernel does not call
> ext4_set_feature_metadata_csum() to enable metadata checksum, and this feature can only be enabled
> by mkfs,turn2fs or change the image directly. So this feature bit will never change once the
> file system is mounted, the second case could never happen ?

This was triggered by a maliciously created file system where the
journal contained a superblock which had the metadata checksum feature
enabled (although the superblock which was visible to the kernel when
it was initially mounted did not have the metadata checksum field
set).

So the file system would get mounted, with metadata_csum not enabled,
so the crc32c checksum was not initialized.  Then the journal replay
would overwrite the superblock with a version that had the
metadata_csum feature set.  And then the next time the kernel tried to
fetch an inode, it would try to check the inode's metadata checksum,
and dereference a NULL pointer.... and boom.

This was found by a researcher that was investigating file system
fuzzing techniques.  So if you have a system with automount enabled,
this is one more way that someone with access to the USB port could
plug in a maliciously crafted file system, and cause the system to
crash, or at least oops.  I don't think *this* particular one could be
exploited to cause a remote execution attack, just a DOS, but it's why
it was assigned a CVE.

> BTW, does this patch need on the old kernel before dec214d00e "ext4: xattr inode deduplication" ?

It's needed on any old kernel which supports the metadata checksum
feature.

Cheers,

					- Ted

Re: Question about commit "ext4: always initialize the crc32c checksum driver"

[zhangyi (F)]

Thanks for your deep explanation, I get it.

Thanks,
Yi.

On 2018/12/14 11:40, Theodore Y. Ts'o Wrote:
> On Thu, Dec 13, 2018 at 03:56:04PM +0800, zhangyi (F) wrote:
>> I am checking a CVE patch a45403b515 "ext4: always initialize the crc32c checksum driver"[1]
>> in CVE-2018-1094[2] recently, and have a question about the commit log in this patch.
>>
>> The patch commit log said:
>>
>>> The extended attribute code now uses the crc32c checksum for hashing
>>> purposes, so we should just always always initialize it.  We also want
>>> to prevent NULL pointer dereferences if one of the metadata checksum
>>> features is enabled after the file sytsem is originally mounted.
>>
>> This first fix is clear. But I don't understand the second fix. IIUC, the kernel does not call
>> ext4_set_feature_metadata_csum() to enable metadata checksum, and this feature can only be enabled
>> by mkfs,turn2fs or change the image directly. So this feature bit will never change once the
>> file system is mounted, the second case could never happen ?
> 
> This was triggered by a maliciously created file system where the
> journal contained a superblock which had the metadata checksum feature
> enabled (although the superblock which was visible to the kernel when
> it was initially mounted did not have the metadata checksum field
> set).
> 
> So the file system would get mounted, with metadata_csum not enabled,
> so the crc32c checksum was not initialized.  Then the journal replay
> would overwrite the superblock with a version that had the
> metadata_csum feature set.  And then the next time the kernel tried to
> fetch an inode, it would try to check the inode's metadata checksum,
> and dereference a NULL pointer.... and boom.
> 
> This was found by a researcher that was investigating file system
> fuzzing techniques.  So if you have a system with automount enabled,
> this is one more way that someone with access to the USB port could
> plug in a maliciously crafted file system, and cause the system to
> crash, or at least oops.  I don't think *this* particular one could be
> exploited to cause a remote execution attack, just a DOS, but it's why
> it was assigned a CVE.
> 
>> BTW, does this patch need on the old kernel before dec214d00e "ext4: xattr inode deduplication" ?
> 
> It's needed on any old kernel which supports the metadata checksum
> feature.
> 
> Cheers,
> 
> 					- Ted
> 
> .
>