From: Alexander Sverdlin <[email protected]>
spi_nor_parse_sfdp() modifies the passed structure so that it points to
itself (params.erase_map.regions to params.erase_map.uniform_region). This
makes it impossible to copy the local struct anywhere else.
Therefore only use memcpy() in backup-restore scenario. The bug may show up
like below:
BUG: unable to handle page fault for address: ffffc90000b377f8
Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 4 PID: 3500 Comm: flashcp Tainted: G O 5.4.53-... #1
...
RIP: 0010:spi_nor_erase+0x8e/0x5c0
Code: 64 24 18 89 db 4d 8b b5 d0 04 00 00 4c 89 64 24 18 4c 89 64 24 20 eb 12 a8 10 0f 85 59 02 00 00 49 83 c6 10 0f 84 4f 02 00 00 <49> 8b 06 48 89 c2 48 83 e2 c0 48 89 d1 49 03 4e 08 48 39 cb 73 d8
RSP: 0018:ffffc9000217fc48 EFLAGS: 00010206
RAX: 0000000000740000 RBX: 0000000000000000 RCX: 0000000000740000
RDX: ffff8884550c9980 RSI: ffff88844f9c0bc0 RDI: ffff88844ede7bb8
RBP: 0000000000740000 R08: ffffffff815bfbe0 R09: ffff88844f9c0bc0
R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000217fc60
R13: ffff88844ede7818 R14: ffffc90000b377f8 R15: 0000000000000000
FS: 00007f4699780500(0000) GS:ffff88846ff00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffc90000b377f8 CR3: 00000004538ee000 CR4: 0000000000340fe0
Call Trace:
part_erase+0x27/0x50
mtdchar_ioctl+0x831/0xba0
? filemap_map_pages+0x186/0x3d0
? do_filp_open+0xad/0x110
? _copy_to_user+0x22/0x30
? cp_new_stat+0x150/0x180
mtdchar_unlocked_ioctl+0x2a/0x40
do_vfs_ioctl+0xa0/0x630
? __do_sys_newfstat+0x3c/0x60
ksys_ioctl+0x70/0x80
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x6a/0x200
? prepare_exit_to_usermode+0x50/0xd0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f46996b6817
Fixes: 1c1d8d98e1c7 ("mtd: spi-nor: Split spi_nor_init_params()")
Cc: [email protected]
Tested-by: Baurzhan Ismagulov <[email protected]>
Co-developed-by: Matija Glavinic Pecotic <[email protected]>
Signed-off-by: Matija Glavinic Pecotic <[email protected]>
Signed-off-by: Alexander Sverdlin <[email protected]>
---
drivers/mtd/spi-nor/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 2add4a0..cce0670 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2701,11 +2701,10 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
- if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+ if (spi_nor_parse_sfdp(nor, nor->params)) {
+ memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
nor->addr_width = 0;
nor->flags &= ~SNOR_F_4B_OPCODES;
- } else {
- memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
}
}
--
2.10.2
On 10/5/20 11:48 AM, Alexander A Sverdlin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> From: Alexander Sverdlin <[email protected]>
>
> spi_nor_parse_sfdp() modifies the passed structure so that it points to
> itself (params.erase_map.regions to params.erase_map.uniform_region). This
> makes it impossible to copy the local struct anywhere else.
>
> Therefore only use memcpy() in backup-restore scenario. The bug may show up
> like below:
>
> BUG: unable to handle page fault for address: ffffc90000b377f8
> Oops: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 4 PID: 3500 Comm: flashcp Tainted: G O 5.4.53-... #1
> ...
> RIP: 0010:spi_nor_erase+0x8e/0x5c0
> Code: 64 24 18 89 db 4d 8b b5 d0 04 00 00 4c 89 64 24 18 4c 89 64 24 20 eb 12 a8 10 0f 85 59 02 00 00 49 83 c6 10 0f 84 4f 02 00 00 <49> 8b 06 48 89 c2 48 83 e2 c0 48 89 d1 49 03 4e 08 48 39 cb 73 d8
> RSP: 0018:ffffc9000217fc48 EFLAGS: 00010206
> RAX: 0000000000740000 RBX: 0000000000000000 RCX: 0000000000740000
> RDX: ffff8884550c9980 RSI: ffff88844f9c0bc0 RDI: ffff88844ede7bb8
> RBP: 0000000000740000 R08: ffffffff815bfbe0 R09: ffff88844f9c0bc0
> R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000217fc60
> R13: ffff88844ede7818 R14: ffffc90000b377f8 R15: 0000000000000000
> FS: 00007f4699780500(0000) GS:ffff88846ff00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffc90000b377f8 CR3: 00000004538ee000 CR4: 0000000000340fe0
> Call Trace:
> part_erase+0x27/0x50
> mtdchar_ioctl+0x831/0xba0
> ? filemap_map_pages+0x186/0x3d0
> ? do_filp_open+0xad/0x110
> ? _copy_to_user+0x22/0x30
> ? cp_new_stat+0x150/0x180
> mtdchar_unlocked_ioctl+0x2a/0x40
> do_vfs_ioctl+0xa0/0x630
> ? __do_sys_newfstat+0x3c/0x60
> ksys_ioctl+0x70/0x80
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x6a/0x200
> ? prepare_exit_to_usermode+0x50/0xd0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f46996b6817
>
> Fixes: 1c1d8d98e1c7 ("mtd: spi-nor: Split spi_nor_init_params()")
I think the correct Fixes tag is:
Fixes: c46872170a54 ("mtd: spi-nor: Move erase_map to 'struct spi_nor_flash_parameter'")
> Cc: [email protected]
> Tested-by: Baurzhan Ismagulov <[email protected]>
> Co-developed-by: Matija Glavinic Pecotic <[email protected]>
> Signed-off-by: Matija Glavinic Pecotic <[email protected]>
> Signed-off-by: Alexander Sverdlin <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2add4a0..cce0670 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2701,11 +2701,10 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>
> memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
>
> - if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> + if (spi_nor_parse_sfdp(nor, nor->params)) {
> + memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> nor->addr_width = 0;
> nor->flags &= ~SNOR_F_4B_OPCODES;
> - } else {
> - memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
neat!
With the Fixes tag fixed, one can add:
Reviewed-by: Tudor Ambarus <[email protected]>
> }
> }
>
> --
> 2.10.2
>
Hello Tudor,
On 07/10/2020 10:48, [email protected] wrote:
>> From: Alexander Sverdlin <[email protected]>
>>
>> spi_nor_parse_sfdp() modifies the passed structure so that it points to
>> itself (params.erase_map.regions to params.erase_map.uniform_region). This
>> makes it impossible to copy the local struct anywhere else.
>>
>> Therefore only use memcpy() in backup-restore scenario. The bug may show up
>> like below:
>>
>> BUG: unable to handle page fault for address: ffffc90000b377f8
>> Oops: 0000 [#1] PREEMPT SMP NOPTI
>> CPU: 4 PID: 3500 Comm: flashcp Tainted: G O 5.4.53-... #1
>> ...
>> RIP: 0010:spi_nor_erase+0x8e/0x5c0
>> Code: 64 24 18 89 db 4d 8b b5 d0 04 00 00 4c 89 64 24 18 4c 89 64 24 20 eb 12 a8 10 0f 85 59 02 00 00 49 83 c6 10 0f 84 4f 02 00 00 <49> 8b 06 48 89 c2 48 83 e2 c0 48 89 d1 49 03 4e 08 48 39 cb 73 d8
>> RSP: 0018:ffffc9000217fc48 EFLAGS: 00010206
>> RAX: 0000000000740000 RBX: 0000000000000000 RCX: 0000000000740000
>> RDX: ffff8884550c9980 RSI: ffff88844f9c0bc0 RDI: ffff88844ede7bb8
>> RBP: 0000000000740000 R08: ffffffff815bfbe0 R09: ffff88844f9c0bc0
>> R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000217fc60
>> R13: ffff88844ede7818 R14: ffffc90000b377f8 R15: 0000000000000000
>> FS: 00007f4699780500(0000) GS:ffff88846ff00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: ffffc90000b377f8 CR3: 00000004538ee000 CR4: 0000000000340fe0
>> Call Trace:
>> part_erase+0x27/0x50
>> mtdchar_ioctl+0x831/0xba0
>> ? filemap_map_pages+0x186/0x3d0
>> ? do_filp_open+0xad/0x110
>> ? _copy_to_user+0x22/0x30
>> ? cp_new_stat+0x150/0x180
>> mtdchar_unlocked_ioctl+0x2a/0x40
>> do_vfs_ioctl+0xa0/0x630
>> ? __do_sys_newfstat+0x3c/0x60
>> ksys_ioctl+0x70/0x80
>> __x64_sys_ioctl+0x16/0x20
>> do_syscall_64+0x6a/0x200
>> ? prepare_exit_to_usermode+0x50/0xd0
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> RIP: 0033:0x7f46996b6817
>>
>> Fixes: 1c1d8d98e1c7 ("mtd: spi-nor: Split spi_nor_init_params()")
> I think the correct Fixes tag is:
> Fixes: c46872170a54 ("mtd: spi-nor: Move erase_map to 'struct spi_nor_flash_parameter'")
yes, I think you are right regarding the exact hash!
Thank you for checking this!
>> Cc: [email protected]
>> Tested-by: Baurzhan Ismagulov <[email protected]>
>> Co-developed-by: Matija Glavinic Pecotic <[email protected]>
>> Signed-off-by: Matija Glavinic Pecotic <[email protected]>
>> Signed-off-by: Alexander Sverdlin <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 2add4a0..cce0670 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2701,11 +2701,10 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>>
>> memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
>>
>> - if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
>> + if (spi_nor_parse_sfdp(nor, nor->params)) {
>> + memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
>> nor->addr_width = 0;
>> nor->flags &= ~SNOR_F_4B_OPCODES;
>> - } else {
>> - memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> neat!
> With the Fixes tag fixed, one can add:
> Reviewed-by: Tudor Ambarus <[email protected]>
--
Best regards,
Alexander Sverdlin.
On Mon, 5 Oct 2020 10:48:03 +0200, Alexander A Sverdlin wrote:
> spi_nor_parse_sfdp() modifies the passed structure so that it points to
> itself (params.erase_map.regions to params.erase_map.uniform_region). This
> makes it impossible to copy the local struct anywhere else.
>
> Therefore only use memcpy() in backup-restore scenario. The bug may show up
> like below:
>
> [...]
Updated Fixes tag as suggested by Tudor.
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks!
[1/1] mtd: spi-nor: Don't copy self-pointing struct around
https://git.kernel.org/mtd/c/69a8eed58c
--
Regards
Vignesh