| Message ID | 20240606131222.1131880-1-linux-mmc@danman.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mmc: core: allow detection of locked cards | expand |
On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > > From: Daniel Kucera <linux-mmc@danman.eu> > > Locked SD card will not reply to SEND_SCR or SD_STATUS commands > so it was failing to initialize previously. When skipped, > the card will get initialized and CMD42 can be sent using > ioctl to unlock the card or remove password protection. > For eMMC, this is not necessary because all initialization > commands are allowed in locked state. > Until unlocked, all read/write calls will timeout. Skipping the commands above, only means the card gets partially initialized. Leaving a card in that state seems fragile. What will happen to upper block layers and filesystems when trying to access it? Several years ago Al Cooper made an attempt [1] to manage the unlocking of the card during initialization, by finding the password through the "keys" subsystem. The series didn't really make it to the upstream kernel, but overall the approach seemed to make sense to me. It should allow us to complete the initialization of the card inside the kernel and prevent unnecessary complexity for userspace to manage. Perhaps you can have a closer look at that series and see if it's possible to update it? Kind regards Uffe [1] https://lore.kernel.org/linux-mmc/1433526147-25941-1-git-send-email-alcooperx@gmail.com/ > > Signed-off-by: Daniel Kucera <linux-mmc@danman.eu> > --- > drivers/mmc/core/sd.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c > index 1c8148cdd..ae821df7d 100644 > --- a/drivers/mmc/core/sd.c > +++ b/drivers/mmc/core/sd.c > @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, > bool reinit) > { > int err; > + u32 card_status; > > - if (!reinit) { > + err = mmc_send_status(card, &card_status); > + if (err){ > + pr_err("%s: unable to get card status\n", mmc_hostname(host)); > + return err; > + } > + > + if (card_status & R1_CARD_IS_LOCKED){ > + pr_warn("%s: card is locked\n", mmc_hostname(host)); > + } > + > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { > /* > * Fetch SCR from card. > */ > -- > 2.34.1 >
On 2024-06-20 14:38, Ulf Hansson wrote: > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >> >> From: Daniel Kucera <linux-mmc@danman.eu> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >> so it was failing to initialize previously. When skipped, >> the card will get initialized and CMD42 can be sent using >> ioctl to unlock the card or remove password protection. >> For eMMC, this is not necessary because all initialization >> commands are allowed in locked state. >> Until unlocked, all read/write calls will timeout. > > Skipping the commands above, only means the card gets partially > initialized. Correct, but it's an improvement in comparison to current state. > Leaving a card in that state seems fragile. Fragile in what sense? Nothing can happen to the card as it is locked. > What will > happen to upper block layers and filesystems when trying to access it? Everything will simply time-out. > > Several years ago Al Cooper made an attempt [1] to manage the > unlocking of the card during initialization, by finding the password > through the "keys" subsystem. The series didn't really make it to the > upstream kernel, but overall the approach seemed to make sense to me. > It should allow us to complete the initialization of the card inside > the kernel and prevent unnecessary complexity for userspace to manage. Yes, he did a great work but obviously no-one got too excited to review/merge his work. His solution was complex. Mine targets the smallest change possible to make it work at least. I wanted to avoid a solution that would be hard to test, review and maintain. Especially when there is only a small interest in the feature. > Perhaps you can have a closer look at that series and see if it's > possible to update it? I don't think I have the skills to revive his work. > > Kind regards > Uffe BR Daniel > > [1] > https://lore.kernel.org/linux-mmc/1433526147-25941-1-git-send-email-alcooperx@gmail.com/ > >> >> Signed-off-by: Daniel Kucera <linux-mmc@danman.eu> >> --- >> drivers/mmc/core/sd.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c >> index 1c8148cdd..ae821df7d 100644 >> --- a/drivers/mmc/core/sd.c >> +++ b/drivers/mmc/core/sd.c >> @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, >> struct mmc_card *card, >> bool reinit) >> { >> int err; >> + u32 card_status; >> >> - if (!reinit) { >> + err = mmc_send_status(card, &card_status); >> + if (err){ >> + pr_err("%s: unable to get card status\n", >> mmc_hostname(host)); >> + return err; >> + } >> + >> + if (card_status & R1_CARD_IS_LOCKED){ >> + pr_warn("%s: card is locked\n", mmc_hostname(host)); >> + } >> + >> + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { >> /* >> * Fetch SCR from card. >> */ >> -- >> 2.34.1 >>
On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> wrote: > > On 2024-06-20 14:38, Ulf Hansson wrote: > > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > >> > >> From: Daniel Kucera <linux-mmc@danman.eu> > >> > >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands > >> so it was failing to initialize previously. When skipped, > >> the card will get initialized and CMD42 can be sent using > >> ioctl to unlock the card or remove password protection. > >> For eMMC, this is not necessary because all initialization > >> commands are allowed in locked state. > >> Until unlocked, all read/write calls will timeout. > > > > Skipping the commands above, only means the card gets partially > > initialized. > > Correct, but it's an improvement in comparison to current state. Not sure I agree with that, sorry. > > > Leaving a card in that state seems fragile. > > Fragile in what sense? Nothing can happen to the card as it is locked. We may end up having a card half-way initialized that we can't really communicate with in a stable manner. From a system point of view, I would be worried. I would rather just power off the card if initialization fails and remove its corresponding device from the system. > > > What will > > happen to upper block layers and filesystems when trying to access it? > > Everything will simply time-out. Yes, but it's uncertain what that could lead to? What will happen with power consumption and power management support, for example. > > > > > Several years ago Al Cooper made an attempt [1] to manage the > > unlocking of the card during initialization, by finding the password > > through the "keys" subsystem. The series didn't really make it to the > > upstream kernel, but overall the approach seemed to make sense to me. > > It should allow us to complete the initialization of the card inside > > the kernel and prevent unnecessary complexity for userspace to manage. > > Yes, he did a great work but obviously no-one got too excited to > review/merge > his work. His solution was complex. It's quite some amount of code. On the other hand, it seemed quite straight forward, not that complex to me. > > Mine targets the smallest change possible to make it work at least. > I wanted to avoid a solution that would be hard to test, review and > maintain. > Especially when there is only a small interest in the feature. > > > Perhaps you can have a closer look at that series and see if it's > > possible to update it? > > I don't think I have the skills to revive his work. I see, maybe we should ping Al and other interested folkz to see if there still is some interest to move this forward? [...] Kind regards Uffe
On 6/20/24 15:32, Ulf Hansson wrote: > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> wrote: >> >> On 2024-06-20 14:38, Ulf Hansson wrote: >>> On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >>>> >>>> From: Daniel Kucera <linux-mmc@danman.eu> >>>> >>>> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >>>> so it was failing to initialize previously. When skipped, >>>> the card will get initialized and CMD42 can be sent using >>>> ioctl to unlock the card or remove password protection. >>>> For eMMC, this is not necessary because all initialization >>>> commands are allowed in locked state. >>>> Until unlocked, all read/write calls will timeout. >>> >>> Skipping the commands above, only means the card gets partially >>> initialized. >> >> Correct, but it's an improvement in comparison to current state. > > Not sure I agree with that, sorry. > >> >>> Leaving a card in that state seems fragile. >> >> Fragile in what sense? Nothing can happen to the card as it is locked. > > We may end up having a card half-way initialized that we can't really > communicate with in a stable manner. From a system point of view, I > would be worried. > > I would rather just power off the card if initialization fails and > remove its corresponding device from the system. > >> >>> What will >>> happen to upper block layers and filesystems when trying to access it? >> >> Everything will simply time-out. > > Yes, but it's uncertain what that could lead to? > > What will happen with power consumption and power management support, > for example. Definitely an aspect that needs to be considered, probably even just powering it off after 10ish seconds would be better, then you still get the chance of unlocking it without having a locked card unknowingly consuming power. Having a saved key and sending that to any card being plugged in seems wrong if you consider security, then again if you consider security you should probably somewhere else than the SD/MMC LOCK/UNLOCK ;) Kind Regards, Christian
On 2024-06-20 16:32, Ulf Hansson wrote: > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> > wrote: >> >> On 2024-06-20 14:38, Ulf Hansson wrote: >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >> >> >> >> From: Daniel Kucera <linux-mmc@danman.eu> >> >> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >> >> so it was failing to initialize previously. When skipped, >> >> the card will get initialized and CMD42 can be sent using >> >> ioctl to unlock the card or remove password protection. >> >> For eMMC, this is not necessary because all initialization >> >> commands are allowed in locked state. >> >> Until unlocked, all read/write calls will timeout. >> > >> > Skipping the commands above, only means the card gets partially >> > initialized. >> >> Correct, but it's an improvement in comparison to current state. > > Not sure I agree with that, sorry. Are you saying that that being able to work with locked card is not an improvement in comparison to not being able to? Or did I misunderstand that? > >> >> > Leaving a card in that state seems fragile. >> >> Fragile in what sense? Nothing can happen to the card as it is locked. > > We may end up having a card half-way initialized that we can't really > communicate with in a stable manner. From a system point of view, I > would be worried. It's not half-way initialized, it's initialized correctly, it's just not using the full potential of the card (higher speeds, etc.). The situation would be the same as it is currently with emmc. Locked emmc gets initialized correctly but reads/writes time-out. What is wrong in having the same result for both sd and emmc? > > I would rather just power off the card if initialization fails and > remove its corresponding device from the system. > >> >> > What will >> > happen to upper block layers and filesystems when trying to access it? >> >> Everything will simply time-out. > > Yes, but it's uncertain what that could lead to? > > What will happen with power consumption and power management support, > for example. Okay, this is a valid concern. Would it be acceptable if we just enabled this "feature" with a module parameter, something like "sd_initialize_locked=1" with default 0? > >> >> > >> > Several years ago Al Cooper made an attempt [1] to manage the >> > unlocking of the card during initialization, by finding the password >> > through the "keys" subsystem. The series didn't really make it to the >> > upstream kernel, but overall the approach seemed to make sense to me. >> > It should allow us to complete the initialization of the card inside >> > the kernel and prevent unnecessary complexity for userspace to manage. >> >> Yes, he did a great work but obviously no-one got too excited to >> review/merge >> his work. His solution was complex. > > It's quite some amount of code. On the other hand, it seemed quite > straight forward, not that complex to me. It doesn't solve the situation when you don't know the password and you just want to erase the card and continue using in. The (un)locking doesn't belong to kernel IMO, if it can be implemented in user-space, it should and it is more flexible that way. > >> >> Mine targets the smallest change possible to make it work at least. >> I wanted to avoid a solution that would be hard to test, review and >> maintain. >> Especially when there is only a small interest in the feature. >> >> > Perhaps you can have a closer look at that series and see if it's >> > possible to update it? >> >> I don't think I have the skills to revive his work. > > I see, maybe we should ping Al and other interested folkz to see if > there still is some interest to move this forward? Okay, Al, are you interested? > > [...] > > Kind regards > Uffe BR Daniel
> On 2024-06-20 16:32, Ulf Hansson wrote: > > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> > > wrote: > >> > >> On 2024-06-20 14:38, Ulf Hansson wrote: > >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > >> >> > >> >> From: Daniel Kucera <linux-mmc@danman.eu> > >> >> > >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands so > >> >> it was failing to initialize previously. When skipped, the card > >> >> will get initialized and CMD42 can be sent using ioctl to unlock > >> >> the card or remove password protection. > >> >> For eMMC, this is not necessary because all initialization > >> >> commands are allowed in locked state. > >> >> Until unlocked, all read/write calls will timeout. > >> > > >> > Skipping the commands above, only means the card gets partially > >> > initialized. > >> > >> Correct, but it's an improvement in comparison to current state. > > > > Not sure I agree with that, sorry. > > Are you saying that that being able to work with locked card is not an > improvement in comparison to not being able to? Or did I misunderstand > that? > > > > >> > >> > Leaving a card in that state seems fragile. > >> > >> Fragile in what sense? Nothing can happen to the card as it is locked. > > > > We may end up having a card half-way initialized that we can't really > > communicate with in a stable manner. From a system point of view, I > > would be worried. Actually, it's what the spec expects - the CARD_IS_LOCKED is carried in CMD7 response. Then the card responds to class 0, class 7, and ACMD41. > > It's not half-way initialized, it's initialized correctly, it's just not using the full > potential of the card (higher speeds, etc.). > > The situation would be the same as it is currently with emmc. Locked emmc > gets initialized correctly but reads/writes time-out. What is wrong in having > the same result for both sd and emmc? > > > > > I would rather just power off the card if initialization fails and > > remove its corresponding device from the system. > > > >> > >> > What will > >> > happen to upper block layers and filesystems when trying to access it? > >> > >> Everything will simply time-out. > > > > Yes, but it's uncertain what that could lead to? > > > > What will happen with power consumption and power management > support, > > for example. > > Okay, this is a valid concern. Would it be acceptable if we just enabled this > "feature" with a module parameter, something like "sd_initialize_locked=1" > with default 0? > > > > >> > >> > > >> > Several years ago Al Cooper made an attempt [1] to manage the > >> > unlocking of the card during initialization, by finding the > >> > password through the "keys" subsystem. The series didn't really > >> > make it to the upstream kernel, but overall the approach seemed to make > sense to me. > >> > It should allow us to complete the initialization of the card > >> > inside the kernel and prevent unnecessary complexity for userspace to > manage. > >> > >> Yes, he did a great work but obviously no-one got too excited to > >> review/merge his work. His solution was complex. > > > > It's quite some amount of code. On the other hand, it seemed quite > > straight forward, not that complex to me. > > It doesn't solve the situation when you don't know the password and you just > want to erase the card and continue using in. The (un)locking doesn't belong > to kernel IMO, if it can be implemented in user-space, it should and it is more > flexible that way. I also see little value in handling the unlocking process in the kernel. I find the proposed approach, e.g. co-exists with its sibling mmc-utils patch, straight forward and simpler. Thanks, Avri > > > > >> > >> Mine targets the smallest change possible to make it work at least. > >> I wanted to avoid a solution that would be hard to test, review and > >> maintain. > >> Especially when there is only a small interest in the feature. > >> > >> > Perhaps you can have a closer look at that series and see if it's > >> > possible to update it? > >> > >> I don't think I have the skills to revive his work. > > > > I see, maybe we should ping Al and other interested folkz to see if > > there still is some interest to move this forward? > > Okay, Al, are you interested? > > > > > [...] > > > > Kind regards > > Uffe > > BR > Daniel >
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index 1c8148cdd..ae821df7d 100644 --- a/drivers/mmc/core/sd.c +++ b/drivers/mmc/core/sd.c @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, bool reinit) { int err; + u32 card_status; - if (!reinit) { + err = mmc_send_status(card, &card_status); + if (err){ + pr_err("%s: unable to get card status\n", mmc_hostname(host)); + return err; + } + + if (card_status & R1_CARD_IS_LOCKED){ + pr_warn("%s: card is locked\n", mmc_hostname(host)); + } + + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { /* * Fetch SCR from card. */