When checking the file name attribute, we want to ensure that it fits
within the bounds of ATTR_RECORD. To do this, we should check
that (attr record + file name offset + file name length) < (attr
record + attr record length).
However, the original check did not include the file name offset in
the calculation. This means that corrupted on-disk metadata might not
caught by the incorrect file name check, and lead to an invalid memory
access.
An example can be seen in the crash report of a memory corruption
error found by Syzbot:
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
Adding the file name offset to the validity check fixes this error and
passes the Syzbot reproducer test.
Reported-by: [email protected]
Tested-by: [email protected]
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/ntfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index f5c058b3192c..4474adb393ca 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
}
file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
le16_to_cpu(attr->data.resident.value_offset));
- p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
+ p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
if (p2 < (u8*)attr || p2 > p)
goto err_corrupt_attr;
/* This attribute is ok, but is it in the $Extend directory? */
--
2.25.1
On 14/6/21 1:05 pm, Desmond Cheong Zhi Xi wrote:
> When checking the file name attribute, we want to ensure that it fits
> within the bounds of ATTR_RECORD. To do this, we should check
> that (attr record + file name offset + file name length) < (attr
> record + attr record length).
>
> However, the original check did not include the file name offset in
> the calculation. This means that corrupted on-disk metadata might not
> caught by the incorrect file name check, and lead to an invalid memory
> access.
>
> An example can be seen in the crash report of a memory corruption
> error found by Syzbot:
> https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
>
> Adding the file name offset to the validity check fixes this error and
> passes the Syzbot reproducer test.
>
> Reported-by: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> fs/ntfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index f5c058b3192c..4474adb393ca 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
> }
> file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
> le16_to_cpu(attr->data.resident.value_offset));
> - p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
> + p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
> if (p2 < (u8*)attr || p2 > p)
> goto err_corrupt_attr;
> /* This attribute is ok, but is it in the $Extend directory? */
>
Hi Anton,
Any chance to review this patch?
Best wishes,
Desmond
Hi Andrew,
Please can you merge this patch? I am also marking it for stable.
Thanks a lot in advance!
Best regards,
Anton
---
When checking the file name attribute, we want to ensure that it fits
within the bounds of ATTR_RECORD. To do this, we should check
that (attr record + file name offset + file name length) < (attr
record + attr record length).
However, the original check did not include the file name offset in
the calculation. This means that corrupted on-disk metadata might not
caught by the incorrect file name check, and lead to an invalid memory
access.
An example can be seen in the crash report of a memory corruption
error found by Syzbot:
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
Adding the file name offset to the validity check fixes this error and
passes the Syzbot reproducer test.
Reported-by: [email protected]
Tested-by: [email protected]
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Acked-by: Anton Altaparmakov <[email protected]>
Cc: [email protected]
---
fs/ntfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index f5c058b3192c..4474adb393ca 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
}
file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
le16_to_cpu(attr->data.resident.value_offset));
- p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
+ p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
if (p2 < (u8*)attr || p2 > p)
goto err_corrupt_attr;
/* This attribute is ok, but is it in the $Extend directory? */
--
2.25.1
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
Hi,
Thanks for the patch! Have asked Andrew to merge it.
Best regards,
Anton
> On 28 Jun 2021, at 03:45, Desmond Cheong Zhi Xi <[email protected]> wrote:
>
> On 14/6/21 1:05 pm, Desmond Cheong Zhi Xi wrote:
>> When checking the file name attribute, we want to ensure that it fits
>> within the bounds of ATTR_RECORD. To do this, we should check
>> that (attr record + file name offset + file name length) < (attr
>> record + attr record length).
>> However, the original check did not include the file name offset in
>> the calculation. This means that corrupted on-disk metadata might not
>> caught by the incorrect file name check, and lead to an invalid memory
>> access.
>> An example can be seen in the crash report of a memory corruption
>> error found by Syzbot:
>> https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
>> Adding the file name offset to the validity check fixes this error and
>> passes the Syzbot reproducer test.
>> Reported-by: [email protected]
>> Tested-by: [email protected]
>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>> ---
>> fs/ntfs/inode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
>> index f5c058b3192c..4474adb393ca 100644
>> --- a/fs/ntfs/inode.c
>> +++ b/fs/ntfs/inode.c
>> @@ -477,7 +477,7 @@ static int ntfs_is_extended_system_file(ntfs_attr_search_ctx *ctx)
>> }
>> file_name_attr = (FILE_NAME_ATTR*)((u8*)attr +
>> le16_to_cpu(attr->data.resident.value_offset));
>> - p2 = (u8*)attr + le32_to_cpu(attr->data.resident.value_length);
>> + p2 = (u8 *)file_name_attr + le32_to_cpu(attr->data.resident.value_length);
>> if (p2 < (u8*)attr || p2 > p)
>> goto err_corrupt_attr;
>> /* This attribute is ok, but is it in the $Extend directory? */
>
> Hi Anton,
>
> Any chance to review this patch?
>
> Best wishes,
> Desmond
--
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer
Hi,
I was just scanning through some older vulnerabilities and came across
CVE-2018-12929, CVE-2018-12930, and CVE-2018-12931, which are all still open
according to linuxkernelcves.com (originally reported against 4.15 [1]). I
looked into the commits in fs/ntfs/ from 4.15 onwards to see if they were just
missed, but I can't spot anything there. RedHat claims to have them fixed in
one of their kernels [2].
Which makes me wonder if the issue fixed here is a duplicate of the any of the
above. Is there a reason I can't find any patches for the original issue in
tree, like the issue only introduced in a custom patchset that Ubuntu/RedHat
were using? Is this thing worth it's own CVE if it's no duplicate?
Greetings,
Eike
1) https://marc.info/?t=152407734400002&r=1&w=2
2) https://access.redhat.com/errata/RHSA-2019:0641
--
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055
emlix - smart embedded open source
On 29/7/21 4:31 pm, Rolf Eike Beer wrote:
> Hi,
>
> I was just scanning through some older vulnerabilities and came across
> CVE-2018-12929, CVE-2018-12930, and CVE-2018-12931, which are all still open
> according to linuxkernelcves.com (originally reported against 4.15 [1]). I
> looked into the commits in fs/ntfs/ from 4.15 onwards to see if they were just
> missed, but I can't spot anything there. RedHat claims to have them fixed in
> one of their kernels [2].
>
> Which makes me wonder if the issue fixed here is a duplicate of the any of the
> above. Is there a reason I can't find any patches for the original issue in
> tree, like the issue only introduced in a custom patchset that Ubuntu/RedHat
> were using? Is this thing worth it's own CVE if it's no duplicate?
>
> Greetings,
>
> Eike
>
> 1) https://marc.info/?t=152407734400002&r=1&w=2
> 2) https://access.redhat.com/errata/RHSA-2019:0641
>
Hi Eike,
Thanks for digging into this. From a first glance, this bug seems most
similar to CVE-2018-12929.
However, from the logs, the root causes are probably different. The
cause of this bug is specifically in the call to
ntfs_is_extended_system_file [1], but from what I can see this is not
the case for CVE-2018-12929. I don't know enough to comment whether it
needs a CVE, but it has been patched on Linux stable (up to 4.4).
It's worth noting that there's another similar bug that was fixed by
Rustam Kovhaev (+cc) in ntfs_read_locked_inode [2]. This may or may not
have been the issue in CVE-2018-12929.
Link:
https://syzkaller.appspot.com/bug?id=a1a1e379b225812688566745c3e2f7242bffc246
[1]
Link:
https://syzkaller.appspot.com/bug?id=933dab9c03ac47a3d09dd4b0563a0a8fcb35f282
[2]
Best wishes,
Desmond