Re: Brainy: Kernel Memory Leak in sdmmc
On Sun, Feb 15, 2015 at 08:53:08PM +, Miod Vallat wrote:
This ought to fix this problem:
This looks reasonable but needs a tweak if you want to keep the existing
behavior. In the existing code, it has this section above the copyout():
rw_enter_write(sc-sc_lock);
if (request == SDIOCEXECMMC)
error = sdmmc_mmc_command(sc, cmd);
else
error = sdmmc_app_command(sc, cmd);
rw_exit(sc-sc_lock);
if (error !cmd.c_error)
cmd.c_error = error;
and returns 0 even if error is non-zero. With this diff, it will either
return the result of those functions or the result of copyout depending
on whether ucmd-c_data is non-zero. Using the result of copyout is
intentional, but not the result from sdmmc_{mmc,app}_command().
Could you set error=0 after the above chunk or use a new variable?
Also, one of the new lines has space indents rather than a tab.
Index: sdmmc.c
===
RCS file: /cvs/src/sys/dev/sdmmc/sdmmc.c,v
retrieving revision 1.36
diff -u -p -r1.36 sdmmc.c
--- sdmmc.c 1 Nov 2014 16:32:06 - 1.36
+++ sdmmc.c 15 Feb 2015 20:52:08 -
@@ -749,7 +749,7 @@ sdmmc_ioctl(struct device *self, u_long
struct sdmmc_command *ucmd;
struct sdmmc_command cmd;
void *data;
- int error;
+ int error = 0;
switch (request) {
#ifdef SDMMC_DEBUG
@@ -784,8 +784,9 @@ sdmmc_ioctl(struct device *self, u_long
M_WAITOK | M_CANFAIL);
if (data == NULL)
return ENOMEM;
- if (copyin(ucmd-c_data, data, ucmd-c_datalen))
- return EFAULT;
+ error = copyin(ucmd-c_data, data, ucmd-c_datalen);
+ if (error != 0)
+ goto exec_done;
cmd.c_data = data;
cmd.c_datalen = ucmd-c_datalen;
@@ -804,10 +805,10 @@ sdmmc_ioctl(struct device *self, u_long
ucmd-c_flags = cmd.c_flags;
ucmd-c_error = cmd.c_error;
- if (ucmd-c_data copyout(data, ucmd-c_data,
- ucmd-c_datalen))
- return EFAULT;
+ if (ucmd-c_data)
+error = copyout(data, ucmd-c_data, ucmd-c_datalen);
+exec_done:
if (ucmd-c_data)
free(data, M_TEMP, 0);
break;
@@ -815,7 +816,7 @@ sdmmc_ioctl(struct device *self, u_long
default:
return ENOTTY;
}
- return 0;
+ return error;
}
#endif
Brainy: Kernel Memory Leak in sdmmc
Hi, I put here a bug among others: -- dev/sdmmc/sdmmc.c -- 783 data = malloc(ucmd-c_datalen, M_TEMP, M_WAITOK | M_CANFAIL); if (data == NULL) return ENOMEM; if (copyin(ucmd-c_data, data, ucmd-c_datalen)) return EFAULT; --- 'data' is leaked. Found by The Brainy Code Scanner. Maxime
Re: Brainy: Kernel Memory Leak in sdmmc
Hi,
I put here a bug among others:
-- dev/sdmmc/sdmmc.c --
783 data = malloc(ucmd-c_datalen, M_TEMP,
M_WAITOK | M_CANFAIL);
if (data == NULL)
return ENOMEM;
if (copyin(ucmd-c_data, data, ucmd-c_datalen))
return EFAULT;
---
'data' is leaked.
This ought to fix this problem:
Index: sdmmc.c
===
RCS file: /cvs/src/sys/dev/sdmmc/sdmmc.c,v
retrieving revision 1.36
diff -u -p -r1.36 sdmmc.c
--- sdmmc.c 1 Nov 2014 16:32:06 - 1.36
+++ sdmmc.c 15 Feb 2015 20:52:08 -
@@ -749,7 +749,7 @@ sdmmc_ioctl(struct device *self, u_long
struct sdmmc_command *ucmd;
struct sdmmc_command cmd;
void *data;
- int error;
+ int error = 0;
switch (request) {
#ifdef SDMMC_DEBUG
@@ -784,8 +784,9 @@ sdmmc_ioctl(struct device *self, u_long
M_WAITOK | M_CANFAIL);
if (data == NULL)
return ENOMEM;
- if (copyin(ucmd-c_data, data, ucmd-c_datalen))
- return EFAULT;
+ error = copyin(ucmd-c_data, data, ucmd-c_datalen);
+ if (error != 0)
+ goto exec_done;
cmd.c_data = data;
cmd.c_datalen = ucmd-c_datalen;
@@ -804,10 +805,10 @@ sdmmc_ioctl(struct device *self, u_long
ucmd-c_flags = cmd.c_flags;
ucmd-c_error = cmd.c_error;
- if (ucmd-c_data copyout(data, ucmd-c_data,
- ucmd-c_datalen))
- return EFAULT;
+ if (ucmd-c_data)
+ error = copyout(data, ucmd-c_data, ucmd-c_datalen);
+exec_done:
if (ucmd-c_data)
free(data, M_TEMP, 0);
break;
@@ -815,7 +816,7 @@ sdmmc_ioctl(struct device *self, u_long
default:
return ENOTTY;
}
- return 0;
+ return error;
}
#endif
Re: Brainy: Kernel Memory Leak in sdmmc
On Sun, Feb 15, 2015 at 08:53:08PM +, Miod Vallat wrote:
This ought to fix this problem:
This looks reasonable but needs a tweak if you want to keep the existing
behavior. In the existing code, it has this section above the copyout():
rw_enter_write(sc-sc_lock);
if (request == SDIOCEXECMMC)
error = sdmmc_mmc_command(sc, cmd);
else
error = sdmmc_app_command(sc, cmd);
rw_exit(sc-sc_lock);
if (error !cmd.c_error)
cmd.c_error = error;
and returns 0 even if error is non-zero. With this diff, it will either
return the result of those functions or the result of copyout depending
on whether ucmd-c_data is non-zero. Using the result of copyout is
intentional, but not the result from sdmmc_{mmc,app}_command().
Could you set error=0 after the above chunk or use a new variable?
Also, one of the new lines has space indents rather than a tab.
Indeed.
Index: sdmmc.c
===
RCS file: /cvs/src/sys/dev/sdmmc/sdmmc.c,v
retrieving revision 1.36
diff -u -p -r1.36 sdmmc.c
--- sdmmc.c 1 Nov 2014 16:32:06 - 1.36
+++ sdmmc.c 16 Feb 2015 05:14:27 -
@@ -749,7 +749,7 @@ sdmmc_ioctl(struct device *self, u_long
struct sdmmc_command *ucmd;
struct sdmmc_command cmd;
void *data;
- int error;
+ int error = 0;
switch (request) {
#ifdef SDMMC_DEBUG
@@ -784,8 +784,9 @@ sdmmc_ioctl(struct device *self, u_long
M_WAITOK | M_CANFAIL);
if (data == NULL)
return ENOMEM;
- if (copyin(ucmd-c_data, data, ucmd-c_datalen))
- return EFAULT;
+ error = copyin(ucmd-c_data, data, ucmd-c_datalen);
+ if (error != 0)
+ goto exec_done;
cmd.c_data = data;
cmd.c_datalen = ucmd-c_datalen;
@@ -804,10 +805,12 @@ sdmmc_ioctl(struct device *self, u_long
ucmd-c_flags = cmd.c_flags;
ucmd-c_error = cmd.c_error;
- if (ucmd-c_data copyout(data, ucmd-c_data,
- ucmd-c_datalen))
- return EFAULT;
+ if (ucmd-c_data)
+ error = copyout(data, ucmd-c_data, ucmd-c_datalen);
+ else
+ error = 0;
+exec_done:
if (ucmd-c_data)
free(data, M_TEMP, 0);
break;
@@ -815,7 +818,7 @@ sdmmc_ioctl(struct device *self, u_long
default:
return ENOTTY;
}
- return 0;
+ return error;
}
#endif
