* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
@ 2018-09-26 11:04 Patrick Delaunay
2018-09-26 14:38 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Delaunay @ 2018-09-26 11:04 UTC (permalink / raw)
To: u-boot
solve data abort for the command "ums 0 ubi 0"
because result of case blk_get_device_part_str() result is OK
but with block_dev = 0 when CONFIG_CMD_UBIFS is activate and
ubi volume is mounted
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
I check and the issue is still present on U-Boot v2018.09,
on my board stm32mp1 (with NAND device and UBI volume):
STM32MP> ubi part UBI
STM32MP> ubifsmount ubi0:boot
STM32MP> ums 0 ubi 0
data abort
pc : [<ffc60abc>] lr : [<ffc60ab3>]
reloc pc : [<c0110abc>] lr : [<c0110ab3>]
sp : fdc3e460 ip : fdc3e518 fp : fdcf06a8
r10: fdcf06b8 r9 : fdc4eed8 r8 : 00000000
r7 : ffce3d84 r6 : fdcf0740 r5 : fdcf0740 r4 : ffce3d88
r3 : 00000000 r2 : 00000000 r1 : 0000003a r0 : 00000000
Flags: nZCv IRQs off FIQs off Mode SVC_32
Code: f04f4628 9b06fd2a bf082800 0800f04f (f5b3695b) Resetting CPU ...
Even If the command is invalid (ums not allowed on ubi device),
the data abort can be avoid by this patch.
Changes in v2:
- Rebase on master branch
cmd/usb_mass_storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c
index 26b41b4c4..a3a338c 100644
--- a/cmd/usb_mass_storage.c
+++ b/cmd/usb_mass_storage.c
@@ -85,7 +85,7 @@ static int ums_init(const char *devtype, const char *devnums_part_str)
partnum = 0;
/* f_mass_storage.c assumes SECTOR_SIZE sectors */
- if (block_dev->blksz != SECTOR_SIZE)
+ if (!block_dev || block_dev->blksz != SECTOR_SIZE)
goto cleanup;
ums_new = realloc(ums, (ums_count + 1) * sizeof(*ums));
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
2018-09-26 11:04 [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev Patrick Delaunay
@ 2018-09-26 14:38 ` Marek Vasut
2018-09-28 9:30 ` Patrick DELAUNAY
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2018-09-26 14:38 UTC (permalink / raw)
To: u-boot
On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
> solve data abort for the command "ums 0 ubi 0"
> because result of case blk_get_device_part_str() result is OK
> but with block_dev = 0 when CONFIG_CMD_UBIFS is activate and
> ubi volume is mounted
I don't quite understand the commit message, can you reword it?
Also, when is this ever called with block_dev == NULL ? What's the
condition to trigger this and why ?
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> I check and the issue is still present on U-Boot v2018.09,
> on my board stm32mp1 (with NAND device and UBI volume):
>
> STM32MP> ubi part UBI
> STM32MP> ubifsmount ubi0:boot
> STM32MP> ums 0 ubi 0
> data abort
> pc : [<ffc60abc>] lr : [<ffc60ab3>]
> reloc pc : [<c0110abc>] lr : [<c0110ab3>]
> sp : fdc3e460 ip : fdc3e518 fp : fdcf06a8
> r10: fdcf06b8 r9 : fdc4eed8 r8 : 00000000
> r7 : ffce3d84 r6 : fdcf0740 r5 : fdcf0740 r4 : ffce3d88
> r3 : 00000000 r2 : 00000000 r1 : 0000003a r0 : 00000000
> Flags: nZCv IRQs off FIQs off Mode SVC_32
> Code: f04f4628 9b06fd2a bf082800 0800f04f (f5b3695b) Resetting CPU ...
>
> Even If the command is invalid (ums not allowed on ubi device),
> the data abort can be avoid by this patch.
>
>
> Changes in v2:
> - Rebase on master branch
>
> cmd/usb_mass_storage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c
> index 26b41b4c4..a3a338c 100644
> --- a/cmd/usb_mass_storage.c
> +++ b/cmd/usb_mass_storage.c
> @@ -85,7 +85,7 @@ static int ums_init(const char *devtype, const char *devnums_part_str)
> partnum = 0;
>
> /* f_mass_storage.c assumes SECTOR_SIZE sectors */
> - if (block_dev->blksz != SECTOR_SIZE)
> + if (!block_dev || block_dev->blksz != SECTOR_SIZE)
> goto cleanup;
>
> ums_new = realloc(ums, (ums_count + 1) * sizeof(*ums));
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
2018-09-26 14:38 ` Marek Vasut
@ 2018-09-28 9:30 ` Patrick DELAUNAY
2018-09-30 8:09 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Patrick DELAUNAY @ 2018-09-28 9:30 UTC (permalink / raw)
To: u-boot
Hi,
> From: Marek Vasut <marex@denx.de>
>
> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
> > solve data abort for the command "ums 0 ubi 0"
> > because result of case blk_get_device_part_str() result is OK but with
> > block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume is
> > mounted
>
> I don't quite understand the commit message, can you reword it?
Ok, I prepare a V3
> Also, when is this ever called with block_dev == NULL ? What's the condition to
> trigger this and why ?
To reproduce the issue you need to have a ubifs already mounted,
for example with the command:
ubi part UBI
ubifsmount ubi0:boot
I investigate the point, the call stack before the crash is caused by the ubi support in
./disk/part.c:425 = blk_get_device_part_str()
Some part of code here are under CONFIG_CMD_UBIFS with the comment :
"ubi goes through a mtd, rathen then through a regular block device"
So the the function return a pointer to disk_partition_t :
info->type = BOOT_PART_TYPE
info->name = "UBI"
but without block device information !
*dev_desc = NULL
So the issue occurs because, when the ubifs volume is mounted,
The code in cmd/usb_mass_storage.c
static int ums_init(const char *devtype, const char *devnums_part_str)
{
...
for (;;) {
...
partnum = blk_get_device_part_str(devtype, devnum_part_str,
&block_dev, &info, 1);
....
// With devnum_part_str = "ubi 0"
// This function don't return a error and return a valid partnum
// So the function continue until block_dev = NULL is used
.....
if (partnum < 0)
goto cleanup;
/* Check if the argument is in legacy format. If yes,
* expose all partitions by setting the partnum = 0
* e.g. ums 0 mmc 0
*/
if (!strchr(devnum_part_str, ':'))
partnum = 0;
/* f_mass_storage.c assumes SECTOR_SIZE sectors */
if (block_dev->blksz != SECTOR_SIZE)
goto cleanup;
=> crash occur here (on my board) because block_dev = NULL
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> > I check and the issue is still present on U-Boot v2018.09, on my board
> > stm32mp1 (with NAND device and UBI volume):
> >
> > STM32MP> ubi part UBI
> > STM32MP> ubifsmount ubi0:boot
> > STM32MP> ums 0 ubi 0
> > data abort
> > pc : [<ffc60abc>] lr : [<ffc60ab3>]
> > reloc pc : [<c0110abc>] lr : [<c0110ab3>]
> > sp : fdc3e460 ip : fdc3e518 fp : fdcf06a8
> > r10: fdcf06b8 r9 : fdc4eed8 r8 : 00000000
> > r7 : ffce3d84 r6 : fdcf0740 r5 : fdcf0740 r4 : ffce3d88
> > r3 : 00000000 r2 : 00000000 r1 : 0000003a r0 : 00000000
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Code: f04f4628 9b06fd2a bf082800 0800f04f (f5b3695b) Resetting CPU ...
> >
> > Even If the command is invalid (ums not allowed on ubi device), the
> > data abort can be avoid by this patch.
> >
> >
> > Changes in v2:
> > - Rebase on master branch
> >
> > cmd/usb_mass_storage.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c index
> > 26b41b4c4..a3a338c 100644
> > --- a/cmd/usb_mass_storage.c
> > +++ b/cmd/usb_mass_storage.c
> > @@ -85,7 +85,7 @@ static int ums_init(const char *devtype, const char
> *devnums_part_str)
> > partnum = 0;
> >
> > /* f_mass_storage.c assumes SECTOR_SIZE sectors */
> > - if (block_dev->blksz != SECTOR_SIZE)
> > + if (!block_dev || block_dev->blksz != SECTOR_SIZE)
> > goto cleanup;
> >
> > ums_new = realloc(ums, (ums_count + 1) * sizeof(*ums));
> >
>
>
> --
> Best regards,
> Marek Vasut
Regards
Patrick
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
2018-09-28 9:30 ` Patrick DELAUNAY
@ 2018-09-30 8:09 ` Marek Vasut
2018-10-02 7:54 ` Patrick DELAUNAY
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2018-09-30 8:09 UTC (permalink / raw)
To: u-boot
On 09/28/2018 11:30 AM, Patrick DELAUNAY wrote:
> Hi,
Hi,
>> From: Marek Vasut <marex@denx.de>
>>
>> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
>>> solve data abort for the command "ums 0 ubi 0"
>>> because result of case blk_get_device_part_str() result is OK but with
>>> block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume is
>>> mounted
>>
>> I don't quite understand the commit message, can you reword it?
>
> Ok, I prepare a V3
Sending a V3 while discussion is ongoing is pointless.
>> Also, when is this ever called with block_dev == NULL ? What's the condition to
>> trigger this and why ?
>
> To reproduce the issue you need to have a ubifs already mounted,
> for example with the command:
> ubi part UBI
> ubifsmount ubi0:boot
>
> I investigate the point, the call stack before the crash is caused by the ubi support in
> ./disk/part.c:425 = blk_get_device_part_str()
>
> Some part of code here are under CONFIG_CMD_UBIFS with the comment :
> "ubi goes through a mtd, rathen then through a regular block device"
>
> So the the function return a pointer to disk_partition_t :
> info->type = BOOT_PART_TYPE
> info->name = "UBI"
> but without block device information !
> *dev_desc = NULL
Would it rather make sense to fix it here , so it has a valid dev_desc ?
> So the issue occurs because, when the ubifs volume is mounted,
> The code in cmd/usb_mass_storage.c
[...]
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
2018-09-30 8:09 ` Marek Vasut
@ 2018-10-02 7:54 ` Patrick DELAUNAY
2018-10-02 11:20 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Patrick DELAUNAY @ 2018-10-02 7:54 UTC (permalink / raw)
To: u-boot
Hi Marek,
> From: Marek Vasut <marex@denx.de>
> Sent: dimanche 30 septembre 2018 10:09
>
> On 09/28/2018 11:30 AM, Patrick DELAUNAY wrote:
> > Hi,
>
> Hi,
>
> >> From: Marek Vasut <marex@denx.de>
> >>
> >> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
> >>> solve data abort for the command "ums 0 ubi 0"
> >>> because result of case blk_get_device_part_str() result is OK but
> >>> with block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume
> >>> is mounted
> >>
> >> I don't quite understand the commit message, can you reword it?
> >
> > Ok, I prepare a V3
>
> Sending a V3 while discussion is ongoing is pointless.
Sorry, I already sent it Friday only with more clear commit message.
Because I really ashamed by of my this fist comment...
Even me, I can't understand that I wrote.
> >> Also, when is this ever called with block_dev == NULL ? What's the
> >> condition to trigger this and why ?
> >
> > To reproduce the issue you need to have a ubifs already mounted, for
> > example with the command:
> > ubi part UBI
> > ubifsmount ubi0:boot
> >
> > I investigate the point, the call stack before the crash is caused by
> > the ubi support in
> > ./disk/part.c:425 = blk_get_device_part_str()
> >
> > Some part of code here are under CONFIG_CMD_UBIFS with the comment :
> > "ubi goes through a mtd, rathen then through a regular block device"
> >
> > So the the function return a pointer to disk_partition_t :
> > info->type = BOOT_PART_TYPE
> > info->name = "UBI"
> > but without block device information !
> > *dev_desc = NULL
>
> Would it rather make sense to fix it here , so it has a valid dev_desc ?
I am not confident that can be done easily, if I correctly understood the code, because
UBI is not really a regular block device but only directly connected to MTD to allow
simple access to generic fs support for ubifs volume.
So NULL block devicedescriptor is "normal" for UBIFS.
To solve the issue at this level, a fake block device should be implemented for the UBI case....
And I don't sure that I can propose something here without breaking all the other commands
using blk_get_device_part_str () function.
See in ./fs/fs.c, the null dev desc is allowed for UBI
struct fstype_info fstypes[]
#ifdef CONFIG_CMD_UBIFS
{
.fstype = FS_TYPE_UBIFS,
.name = "ubifs",
.null_dev_desc_ok = true,
> > So the issue occurs because, when the ubifs volume is mounted, The
> > code in cmd/usb_mass_storage.c
> [...]
>
> --
> Best regards,
> Marek Vasut
Regards
Patrick
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev
2018-10-02 7:54 ` Patrick DELAUNAY
@ 2018-10-02 11:20 ` Marek Vasut
0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2018-10-02 11:20 UTC (permalink / raw)
To: u-boot
On 10/02/2018 09:54 AM, Patrick DELAUNAY wrote:
> Hi Marek,
Hi,
>> From: Marek Vasut <marex@denx.de>
>> Sent: dimanche 30 septembre 2018 10:09
>>
>> On 09/28/2018 11:30 AM, Patrick DELAUNAY wrote:
>>> Hi,
>>
>> Hi,
>>
>>>> From: Marek Vasut <marex@denx.de>
>>>>
>>>> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
>>>>> solve data abort for the command "ums 0 ubi 0"
>>>>> because result of case blk_get_device_part_str() result is OK but
>>>>> with block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume
>>>>> is mounted
>>>>
>>>> I don't quite understand the commit message, can you reword it?
>>>
>>> Ok, I prepare a V3
>>
>> Sending a V3 while discussion is ongoing is pointless.
>
> Sorry, I already sent it Friday only with more clear commit message.
> Because I really ashamed by of my this fist comment...
> Even me, I can't understand that I wrote.
Happens to everyone.
>>>> Also, when is this ever called with block_dev == NULL ? What's the
>>>> condition to trigger this and why ?
>>>
>>> To reproduce the issue you need to have a ubifs already mounted, for
>>> example with the command:
>>> ubi part UBI
>>> ubifsmount ubi0:boot
>>>
>>> I investigate the point, the call stack before the crash is caused by
>>> the ubi support in
>>> ./disk/part.c:425 = blk_get_device_part_str()
>>>
>>> Some part of code here are under CONFIG_CMD_UBIFS with the comment :
>>> "ubi goes through a mtd, rathen then through a regular block device"
>>>
>>> So the the function return a pointer to disk_partition_t :
>>> info->type = BOOT_PART_TYPE
>>> info->name = "UBI"
>>> but without block device information !
>>> *dev_desc = NULL
>>
>> Would it rather make sense to fix it here , so it has a valid dev_desc ?
>
> I am not confident that can be done easily, if I correctly understood the code, because
> UBI is not really a regular block device but only directly connected to MTD to allow
> simple access to generic fs support for ubifs volume.
>
> So NULL block devicedescriptor is "normal" for UBIFS.
>
> To solve the issue at this level, a fake block device should be implemented for the UBI case....
> And I don't sure that I can propose something here without breaking all the other commands
> using blk_get_device_part_str () function.
>
> See in ./fs/fs.c, the null dev desc is allowed for UBI
>
> struct fstype_info fstypes[]
>
> #ifdef CONFIG_CMD_UBIFS
> {
> .fstype = FS_TYPE_UBIFS,
> .name = "ubifs",
> .null_dev_desc_ok = true,
>
>>> So the issue occurs because, when the ubifs volume is mounted, The
>>> code in cmd/usb_mass_storage.c
>> [...]
Thanks for clarifying.
Simon, thoughts ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-02 11:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-26 11:04 [U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev Patrick Delaunay
2018-09-26 14:38 ` Marek Vasut
2018-09-28 9:30 ` Patrick DELAUNAY
2018-09-30 8:09 ` Marek Vasut
2018-10-02 7:54 ` Patrick DELAUNAY
2018-10-02 11:20 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.