2006-12-04 20:09:01

by Anderson Briglia

[permalink] [raw]
Subject: [PATCH 2/4] Add MMC Password Protection (lock/unlock) support V8: mmc_key_retention.diff

Implement key retention operations.

Signed-off-by: Carlos Eduardo Aguiar <carlos.aguiar <at> indt.org.br>
Signed-off-by: Anderson Lizardo <anderson.lizardo <at> indt.org.br>
Signed-off-by: Anderson Briglia <anderson.briglia <at> indt.org.br>

Index: linux-linus-2.6/drivers/mmc/Kconfig
===================================================================
--- linux-linus-2.6.orig/drivers/mmc/Kconfig 2006-12-04 14:57:42.000000000 -0400
+++ linux-linus-2.6/drivers/mmc/Kconfig 2006-12-04 15:26:05.000000000 -0400
@@ -19,6 +19,19 @@ config MMC_DEBUG
This is an option for use by developers; most people should
say N here. This enables MMC core and driver debugging.

+config MMC_PASSWORDS
+ boolean "MMC card lock/unlock passwords (EXPERIMENTAL)"
+ depends on MMC && EXPERIMENTAL
+ select KEYS
+ help
+ Say Y here to enable the use of passwords to lock and unlock
+ MMC cards. This uses the access key retention support, using
+ request_key to look up the key associated with each card.
+
+ For example, if you have an MMC card that was locked using
+ Symbian OS on your cell phone, you won't be able to read it
+ on Linux without this support.
+
config MMC_BLOCK
tristate "MMC block device driver"
depends on MMC && BLOCK
Index: linux-linus-2.6/drivers/mmc/mmc.h
===================================================================
--- linux-linus-2.6.orig/drivers/mmc/mmc.h 2006-11-22 10:44:30.000000000 -0400
+++ linux-linus-2.6/drivers/mmc/mmc.h 2006-12-04 15:34:07.000000000 -0400
@@ -19,6 +19,14 @@ int mmc_add_host_sysfs(struct mmc_host *
void mmc_remove_host_sysfs(struct mmc_host *host);
void mmc_free_host_sysfs(struct mmc_host *host);

+/* core-internal data */
+extern struct key_type mmc_key_type;
+struct mmc_key_payload {
+ struct rcu_head rcu; /* RCU destructor */
+ unsigned short datalen; /* length of this data */
+ char data[0]; /* actual data */
+};
+
int mmc_schedule_work(struct work_struct *work);
int mmc_schedule_delayed_work(struct work_struct *work, unsigned long delay);
void mmc_flush_scheduled_work(void);
Index: linux-linus-2.6/drivers/mmc/mmc_sysfs.c
===================================================================
--- linux-linus-2.6.orig/drivers/mmc/mmc_sysfs.c 2006-12-04 15:16:57.000000000 -0400
+++ linux-linus-2.6/drivers/mmc/mmc_sysfs.c 2006-12-04 15:34:03.000000000 -0400
@@ -2,6 +2,8 @@
* linux/drivers/mmc/mmc_sysfs.c
*
* Copyright (C) 2003 Russell King, All Rights Reserved.
+ * MMC password protection (C) 2006 Instituto Nokia de Tecnologia (INdT),
+ * All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -14,6 +16,7 @@
#include <linux/device.h>
#include <linux/idr.h>
#include <linux/workqueue.h>
+#include <linux/key.h>

#include <linux/mmc/card.h>
#include <linux/mmc/host.h>
@@ -266,6 +269,71 @@ static struct class mmc_host_class = {
static DEFINE_IDR(mmc_host_idr);
static DEFINE_SPINLOCK(mmc_host_lock);

+#ifdef CONFIG_MMC_PASSWORDS
+
+#define MMC_KEYLEN_MAXBYTES 32
+
+static int mmc_key_instantiate(struct key *key, const void *data, size_t datalen)
+{
+ struct mmc_key_payload *mpayload, *zap;
+ int ret;
+
+ zap = NULL;
+ ret = -EINVAL;
+ if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data) {
+ pr_debug("Invalid data\n");
+ goto error;
+ }
+
+ ret = key_payload_reserve(key, datalen);
+ if (ret < 0) {
+ pr_debug("ret = %d\n", ret);
+ goto error;
+ }
+
+ ret = -ENOMEM;
+ mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
+ if (!mpayload) {
+ pr_debug("Unable to allocate mpayload structure\n");
+ goto error;
+ }
+ mpayload->datalen = datalen;
+ memcpy(mpayload->data, data, datalen);
+
+ rcu_assign_pointer(key->payload.data, mpayload);
+
+ /* ret = 0 if there is no error */
+ ret = 0;
+
+error:
+ return ret;
+}
+
+static int mmc_key_match(const struct key *key, const void *description)
+{
+ return strcmp(key->description, description) == 0;
+}
+
+/*
+ * dispose of the data dangling from the corpse of a mmc key
+ */
+static void mmc_key_destroy(struct key *key)
+{
+ struct mmc_key_payload *mpayload = key->payload.data;
+
+ kfree(mpayload);
+}
+
+struct key_type mmc_key_type = {
+ .name = "mmc",
+ .def_datalen = MMC_KEYLEN_MAXBYTES,
+ .instantiate = mmc_key_instantiate,
+ .match = mmc_key_match,
+ .destroy = mmc_key_destroy,
+};
+
+#endif
+
/*
* Internal function. Allocate a new MMC host.
*/
@@ -363,16 +431,31 @@ static int __init mmc_init(void)
return -ENOMEM;

ret = bus_register(&mmc_bus_type);
- if (ret == 0) {
- ret = class_register(&mmc_host_class);
- if (ret)
- bus_unregister(&mmc_bus_type);
+ if (ret)
+ goto error_bus;
+ ret = class_register(&mmc_host_class);
+ if (ret)
+ goto error_class;
+#ifdef CONFIG_MMC_PASSWORDS
+ ret = register_key_type(&mmc_key_type);
+ if (ret) {
+ class_unregister(&mmc_host_class);
+ goto error_class;
}
+#endif
+ return 0;
+
+error_class:
+ bus_unregister(&mmc_bus_type);
+error_bus:
return ret;
}

static void __exit mmc_exit(void)
{
+#ifdef CONFIG_MMC_PASSWORDS
+ unregister_key_type(&mmc_key_type);
+#endif
class_unregister(&mmc_host_class);
bus_unregister(&mmc_bus_type);
destroy_workqueue(workqueue);


Attachments:
mmc_key_retention.diff (5.14 kB)

2006-12-13 15:03:13

by Frank Seidel

[permalink] [raw]
Subject: Re: [PATCH 2/4] Add MMC Password Protection (lock/unlock) support V8: mmc_key_retention.diff

Quoting Anderson Briglia <[email protected]>:
> [...]
Hi,
thats really cool stuff you're providing with your patches. :)
I have some feedback or questions some parts here.
But as i just started trying to get into kernelhacking you probably
better don't take my notes to serious, please.

> Index: linux-linus-2.6/drivers/mmc/mmc_sysfs.c
> ===================================================================
> --- linux-linus-2.6.orig/drivers/mmc/mmc_sysfs.c 2006-12-04 [...]
> +static int mmc_key_instantiate(struct key *key, const void *data,
> size_t datalen)
> +{
> + struct mmc_key_payload *mpayload, *zap;
> + int ret;
> +
> + zap = NULL;
What is zap here for? future use?
And wouldn't it be good to also initialize mplayload here?

> + ret = -EINVAL;
Is there a special reason why you already assign the errors to the
return value variable before its clear that the assignment is needed?


> + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data) {
Isn't the last "|| !data" redundant as you already tested if datalen ==0?

> + pr_debug("Invalid data\n");
> + goto error;
> + }
> +
> + ret = key_payload_reserve(key, datalen);
> + if (ret < 0) {
> + pr_debug("ret = %d\n", ret);
> + goto error;
> + }
> +
> + ret = -ENOMEM;
Same as above: Why do you in any case want to assign it here?

> + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
I may be totally wrong, but is dereferencing a not initialized pointer
(even just for using sizeof) really ok? Wouldn't it be safer to use
a sizeof(struct mmc_key_payload) here?

Thanks,
Frank


2006-12-14 14:38:00

by Anderson Briglia

[permalink] [raw]
Subject: Re: [PATCH 2/4] Add MMC Password Protection (lock/unlock) support V8: mmc_key_retention.diff

Hi,

ext Frank Seidel wrote:
> Quoting Anderson Briglia <[email protected]>:
>> [...]
> Hi,
> thats really cool stuff you're providing with your patches. :)
> I have some feedback or questions some parts here.
> But as i just started trying to get into kernelhacking you probably
> better don't take my notes to serious, please.

All comments are welcome, thanks for the revision. :)

>
>> Index: linux-linus-2.6/drivers/mmc/mmc_sysfs.c
>> ===================================================================
>> --- linux-linus-2.6.orig/drivers/mmc/mmc_sysfs.c 2006-12-04 [...]
>> +static int mmc_key_instantiate(struct key *key, const void *data,
>> size_t datalen)
>> +{
>> + struct mmc_key_payload *mpayload, *zap;
>> + int ret;
>> +
>> + zap = NULL;
> What is zap here for? future use?
> And wouldn't it be good to also initialize mplayload here?

The code was based on code presents at security/keys/user_defined.c. This is the reason of why the MMC PWD code was
implemented using this returns types and others implementations.
That file (user_defined.c) implements generic functions to handle keys inside the kernel, using the Kernel Key Retention
Service. Maybe you can take a look there, :).
That zap variable was used to expand the key payload when a new password exceeded a previous configured size. But the
Kernel Key Retention Service has changed and that zap variable is not used on key_instantiate function implemented at
user_defined.c, anymore. I'll update the MMC PWD code.

>
>> + ret = -EINVAL;
> Is there a special reason why you already assign the errors to the
> return value variable before its clear that the assignment is needed?

See the reply above.
>
>
>> + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data) {
> Isn't the last "|| !data" redundant as you already tested if datalen ==0?

data = 0 is different from !data.

>
>> + pr_debug("Invalid data\n");
>> + goto error;
>> + }
>> +
>> + ret = key_payload_reserve(key, datalen);
>> + if (ret < 0) {
>> + pr_debug("ret = %d\n", ret);
>> + goto error;
>> + }
>> +
>> + ret = -ENOMEM;
> Same as above: Why do you in any case want to assign it here?

Reply above.
>
>> + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
> I may be totally wrong, but is dereferencing a not initialized pointer
> (even just for using sizeof) really ok?

Yes. I believe sizeof is a compiler operation and it does not access the data pointed by that pointer, it access just
the type of the pointer.

Wouldn't it be safer to use
> a sizeof(struct mmc_key_payload) here?

I believe there is no difference on using this one and that other declaration.

Thanks,

Anderson Briglia

2006-12-15 17:30:31

by Pierre Ossman

[permalink] [raw]
Subject: Re: [PATCH 2/4] Add MMC Password Protection (lock/unlock) support V8: mmc_key_retention.diff

Anderson Briglia wrote:
> The code was based on code presents at security/keys/user_defined.c. This is the reason of why the MMC PWD code was
> implemented using this returns types and others implementations.
> That file (user_defined.c) implements generic functions to handle keys inside the kernel, using the Kernel Key Retention
> Service. Maybe you can take a look there, :).
> That zap variable was used to expand the key payload when a new password exceeded a previous configured size. But the
> Kernel Key Retention Service has changed and that zap variable is not used on key_instantiate function implemented at
> user_defined.c, anymore. I'll update the MMC PWD code.
>
>

Patches look ok, and I'll commit them once you send me this last fix.

> Yes. I believe sizeof is a compiler operation and it does not access the data pointed by that pointer, it access just
> the type of the pointer.
>
>

Yes, sizeof() is compile time and completely safe in this regard.

Rgds

--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2006-12-15 18:53:47

by Anderson Briglia

[permalink] [raw]
Subject: Re: [PATCH 2/4] Add MMC Password Protection (lock/unlock) support V8: mmc_key_retention.diff

ext Pierre Ossman wrote:
[...]
> Patches look ok, and I'll commit them once you send me this last fix.

Please, see the V9 series version, this bug was fixed there. Once we had some changes on the MMC code, I updated the MMC
PWD code according to the latest mainline code and decided to send another set.

Best Regards,


Anderson Briglia