| Message ID | 20201005084803.23460-1-alexander.sverdlin@nokia.com |
|---|---|
| State | Accepted |
| Commit | 69a8eed58cc09aea3b01a64997031dd5d3c02c07 |
| Headers | show |
| Series | mtd: spi-nor: Don't copy self-pointing struct around | expand |
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 <alexander.sverdlin@nokia.com> > > 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: stable@vger.kernel.org > Tested-by: Baurzhan Ismagulov <ibr@radix50.net> > Co-developed-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com> > Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > 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 <tudor.ambarus@microchip.com> > } > } > > -- > 2.10.2 >
Hello Tudor, On 07/10/2020 10:48, Tudor.Ambarus@microchip.com wrote: >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> >> 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: stable@vger.kernel.org >> Tested-by: Baurzhan Ismagulov <ibr@radix50.net> >> Co-developed-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com> >> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> --- >> 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 <tudor.ambarus@microchip.com>
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)); } }