Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 04:35:41PM +0100, Tobias Heider wrote: > On Mon, Jan 10, 2022 at 04:20:36PM +0100, Stefan Sperling wrote: > > On Mon, Jan 10, 2022 at 03:50:45PM +0100, Tobias Heider wrote: > > > Makes sense. I also fixed the one in sdmmc_mem_send_cxd_data(). > > > > Doesn't build here, there a few errors like this: > > > > /usr/src/sys/dev/sdmmc/sdmmc_mem.c:483:1: error: unused label 'out' > > [-Werror,-Wu > > nused-label] > > > > I like Visa's idea of using early 'return ENOMEM' instead of goto. > > I removed the unused goto labels and cleaned up the error handling. > We don't need to check for (ptr != NULL) and in one case we can > merge two 'if (error == 0)' blocks. > > ok? OK visa@
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 04:35:41PM +0100, Tobias Heider wrote:
> On Mon, Jan 10, 2022 at 04:20:36PM +0100, Stefan Sperling wrote:
> > On Mon, Jan 10, 2022 at 03:50:45PM +0100, Tobias Heider wrote:
> > > Makes sense. I also fixed the one in sdmmc_mem_send_cxd_data().
> >
> > Doesn't build here, there a few errors like this:
> >
> > /usr/src/sys/dev/sdmmc/sdmmc_mem.c:483:1: error: unused label 'out'
> > [-Werror,-Wu
> > nused-label]
> >
> > I like Visa's idea of using early 'return ENOMEM' instead of goto.
>
> I removed the unused goto labels and cleaned up the error handling.
> We don't need to check for (ptr != NULL) and in one case we can
> merge two 'if (error == 0)' blocks.
>
> ok?
ok stsp@
> Index: sdmmc_mem.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/dev/sdmmc/sdmmc_mem.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 sdmmc_mem.c
> --- sdmmc_mem.c 27 Mar 2021 14:36:28 - 1.36
> +++ sdmmc_mem.c 10 Jan 2022 05:20:37 -
> @@ -466,7 +466,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *s
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> if (ptr == NULL)
> - goto out;
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -480,9 +480,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *s
> if (error == 0)
> memcpy(scr, ptr, datalen);
>
> -out:
> - if (ptr != NULL)
> - free(ptr, M_DEVBUF, datalen);
> + free(ptr, M_DEVBUF, datalen);
>
> return error;
> }
> @@ -528,10 +526,8 @@ sdmmc_mem_send_cxd_data(struct sdmmc_sof
> int error = 0;
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL) {
> - error = ENOMEM;
> - goto out;
> - }
> + if (ptr == NULL)
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -549,9 +545,7 @@ sdmmc_mem_send_cxd_data(struct sdmmc_sof
> if (error == 0)
> memcpy(data, ptr, datalen);
>
> -out:
> - if (ptr != NULL)
> - free(ptr, M_DEVBUF, datalen);
> + free(ptr, M_DEVBUF, datalen);
>
> return error;
> }
> @@ -608,7 +602,7 @@ sdmmc_mem_sd_switch(struct sdmmc_functio
>
> ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
> if (ptr == NULL)
> - goto out;
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -620,15 +614,12 @@ sdmmc_mem_sd_switch(struct sdmmc_functio
> cmd.c_flags = SCF_CMD_ADTC | SCF_CMD_READ | SCF_RSP_R1;
>
> error = sdmmc_mmc_command(sc, );
> - if (error == 0)
> + if (error == 0) {
> memcpy(status, ptr, statlen);
> -
> -out:
> - if (ptr != NULL)
> - free(ptr, M_DEVBUF, statlen);
> -
> - if (error == 0)
> sdmmc_be512_to_bitfield512(status);
> + }
> +
> + free(ptr, M_DEVBUF, statlen);
>
> return error;
> }
>
>
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 04:20:36PM +0100, Stefan Sperling wrote:
> On Mon, Jan 10, 2022 at 03:50:45PM +0100, Tobias Heider wrote:
> > Makes sense. I also fixed the one in sdmmc_mem_send_cxd_data().
>
> Doesn't build here, there a few errors like this:
>
> /usr/src/sys/dev/sdmmc/sdmmc_mem.c:483:1: error: unused label 'out'
> [-Werror,-Wu
> nused-label]
>
> I like Visa's idea of using early 'return ENOMEM' instead of goto.
I removed the unused goto labels and cleaned up the error handling.
We don't need to check for (ptr != NULL) and in one case we can
merge two 'if (error == 0)' blocks.
ok?
Index: sdmmc_mem.c
===
RCS file: /mount/openbsd/cvs/src/sys/dev/sdmmc/sdmmc_mem.c,v
retrieving revision 1.36
diff -u -p -r1.36 sdmmc_mem.c
--- sdmmc_mem.c 27 Mar 2021 14:36:28 - 1.36
+++ sdmmc_mem.c 10 Jan 2022 05:20:37 -
@@ -466,7 +466,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *s
ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
if (ptr == NULL)
- goto out;
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -480,9 +480,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *s
if (error == 0)
memcpy(scr, ptr, datalen);
-out:
- if (ptr != NULL)
- free(ptr, M_DEVBUF, datalen);
+ free(ptr, M_DEVBUF, datalen);
return error;
}
@@ -528,10 +526,8 @@ sdmmc_mem_send_cxd_data(struct sdmmc_sof
int error = 0;
ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
- if (ptr == NULL) {
- error = ENOMEM;
- goto out;
- }
+ if (ptr == NULL)
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -549,9 +545,7 @@ sdmmc_mem_send_cxd_data(struct sdmmc_sof
if (error == 0)
memcpy(data, ptr, datalen);
-out:
- if (ptr != NULL)
- free(ptr, M_DEVBUF, datalen);
+ free(ptr, M_DEVBUF, datalen);
return error;
}
@@ -608,7 +602,7 @@ sdmmc_mem_sd_switch(struct sdmmc_functio
ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
if (ptr == NULL)
- goto out;
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -620,15 +614,12 @@ sdmmc_mem_sd_switch(struct sdmmc_functio
cmd.c_flags = SCF_CMD_ADTC | SCF_CMD_READ | SCF_RSP_R1;
error = sdmmc_mmc_command(sc, );
- if (error == 0)
+ if (error == 0) {
memcpy(status, ptr, statlen);
-
-out:
- if (ptr != NULL)
- free(ptr, M_DEVBUF, statlen);
-
- if (error == 0)
sdmmc_be512_to_bitfield512(status);
+ }
+
+ free(ptr, M_DEVBUF, statlen);
return error;
}
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 03:50:45PM +0100, Tobias Heider wrote:
> Makes sense. I also fixed the one in sdmmc_mem_send_cxd_data().
Doesn't build here, there a few errors like this:
/usr/src/sys/dev/sdmmc/sdmmc_mem.c:483:1: error: unused label 'out' [-Werror,-Wu
nused-label]
I like Visa's idea of using early 'return ENOMEM' instead of goto.
> diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
> index fae8d63912d..d46b1d612be 100644
> --- a/sys/dev/sdmmc/sdmmc_mem.c
> +++ b/sys/dev/sdmmc/sdmmc_mem.c
> @@ -466,7 +466,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> if (ptr == NULL)
> - goto out;
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -528,10 +528,8 @@ sdmmc_mem_send_cxd_data(struct sdmmc_softc *sc, int
> opcode, void *data,
> int error = 0;
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL) {
> - error = ENOMEM;
> - goto out;
> - }
> + if (ptr == NULL)
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -608,7 +606,7 @@ sdmmc_mem_sd_switch(struct sdmmc_function *sf, int mode,
> int group,
>
> ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
> if (ptr == NULL)
> - goto out;
> + return ENOMEM;
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
>
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 02:39:58PM +, Visa Hankala wrote:
> On Mon, Jan 10, 2022 at 03:21:49PM +0100, Tobias Heider wrote:
> > On Mon, Jan 10, 2022 at 01:41:53PM +, Visa Hankala wrote:
> > > On Mon, Jan 10, 2022 at 01:12:10PM +0100, Tobias Heider wrote:
> > > > sdmmc_mem_send_scr() tries to malloc() with M_NOWAIT and returns 0 on
> > > > error, which leads to sdmmc_mem_sd_init() passing uninitialized stack
> > > > memory to sdmmc_mem_decode_scr().
> > > > The diff below makes sdmmc_mem_send_scr() return ENOMEM if malloc fails.
> > > >
> > > > ok?
> > >
> > > OK visa@
> > >
> > > Isn't there a similar problem with M_NOWAIT in sdmmc_mem_sd_switch()?
> > >
> >
> > Right, here's an updated diff that fixes both.
>
> Looks better. However, could the error branches return ENOMEM directly
> instead of using goto out?
Makes sense. I also fixed the one in sdmmc_mem_send_cxd_data().
diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
index fae8d63912d..d46b1d612be 100644
--- a/sys/dev/sdmmc/sdmmc_mem.c
+++ b/sys/dev/sdmmc/sdmmc_mem.c
@@ -466,7 +466,7 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
if (ptr == NULL)
- goto out;
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -528,10 +528,8 @@ sdmmc_mem_send_cxd_data(struct sdmmc_softc *sc, int
opcode, void *data,
int error = 0;
ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
- if (ptr == NULL) {
- error = ENOMEM;
- goto out;
- }
+ if (ptr == NULL)
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -608,7 +606,7 @@ sdmmc_mem_sd_switch(struct sdmmc_function *sf, int mode,
int group,
ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
if (ptr == NULL)
- goto out;
+ return ENOMEM;
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 03:21:49PM +0100, Tobias Heider wrote:
> On Mon, Jan 10, 2022 at 01:41:53PM +, Visa Hankala wrote:
> > On Mon, Jan 10, 2022 at 01:12:10PM +0100, Tobias Heider wrote:
> > > sdmmc_mem_send_scr() tries to malloc() with M_NOWAIT and returns 0 on
> > > error, which leads to sdmmc_mem_sd_init() passing uninitialized stack
> > > memory to sdmmc_mem_decode_scr().
> > > The diff below makes sdmmc_mem_send_scr() return ENOMEM if malloc fails.
> > >
> > > ok?
> >
> > OK visa@
> >
> > Isn't there a similar problem with M_NOWAIT in sdmmc_mem_sd_switch()?
> >
>
> Right, here's an updated diff that fixes both.
Looks better. However, could the error branches return ENOMEM directly
instead of using goto out?
> diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
> index fae8d63912d..dfb72ec4bf8 100644
> --- a/sys/dev/sdmmc/sdmmc_mem.c
> +++ b/sys/dev/sdmmc/sdmmc_mem.c
> @@ -465,8 +465,10 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
> int error = 0;
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL)
> + if (ptr == NULL) {
> + error = ENOMEM;
> goto out;
> + }
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
> @@ -607,8 +609,10 @@ sdmmc_mem_sd_switch(struct sdmmc_function *sf, int mode,
> int group,
> gsft = (group - 1) << 2;
>
> ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL)
> + if (ptr == NULL) {
> + error = ENOMEM;
> goto out;
> + }
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
>
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 01:41:53PM +, Visa Hankala wrote:
> On Mon, Jan 10, 2022 at 01:12:10PM +0100, Tobias Heider wrote:
> > sdmmc_mem_send_scr() tries to malloc() with M_NOWAIT and returns 0 on
> > error, which leads to sdmmc_mem_sd_init() passing uninitialized stack
> > memory to sdmmc_mem_decode_scr().
> > The diff below makes sdmmc_mem_send_scr() return ENOMEM if malloc fails.
> >
> > ok?
>
> OK visa@
>
> Isn't there a similar problem with M_NOWAIT in sdmmc_mem_sd_switch()?
>
Right, here's an updated diff that fixes both.
diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
index fae8d63912d..dfb72ec4bf8 100644
--- a/sys/dev/sdmmc/sdmmc_mem.c
+++ b/sys/dev/sdmmc/sdmmc_mem.c
@@ -465,8 +465,10 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
int error = 0;
ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
- if (ptr == NULL)
+ if (ptr == NULL) {
+ error = ENOMEM;
goto out;
+ }
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
@@ -607,8 +609,10 @@ sdmmc_mem_sd_switch(struct sdmmc_function *sf, int mode,
int group,
gsft = (group - 1) << 2;
ptr = malloc(statlen, M_DEVBUF, M_NOWAIT | M_ZERO);
- if (ptr == NULL)
+ if (ptr == NULL) {
+ error = ENOMEM;
goto out;
+ }
memset(, 0, sizeof(cmd));
cmd.c_data = ptr;
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 01:12:10PM +0100, Tobias Heider wrote:
> sdmmc_mem_send_scr() tries to malloc() with M_NOWAIT and returns 0 on
> error, which leads to sdmmc_mem_sd_init() passing uninitialized stack
> memory to sdmmc_mem_decode_scr().
> The diff below makes sdmmc_mem_send_scr() return ENOMEM if malloc fails.
>
> ok?
OK visa@
Isn't there a similar problem with M_NOWAIT in sdmmc_mem_sd_switch()?
> diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
> index fae8d63912d..715c412e6ea 100644
> --- a/sys/dev/sdmmc/sdmmc_mem.c
> +++ b/sys/dev/sdmmc/sdmmc_mem.c
> @@ -465,8 +465,10 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
> int error = 0;
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL)
> + if (ptr == NULL) {
> + error = ENOMEM;
> goto out;
> + }
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
>
Re: sdmmc: fix malloc error handling in sdmmc_mem_send_scr()
On Mon, Jan 10, 2022 at 01:12:10PM +0100, Tobias Heider wrote:
> sdmmc_mem_send_scr() tries to malloc() with M_NOWAIT and returns 0 on
> error, which leads to sdmmc_mem_sd_init() passing uninitialized stack
> memory to sdmmc_mem_decode_scr().
> The diff below makes sdmmc_mem_send_scr() return ENOMEM if malloc fails.
>
> ok?
Nice catch. ok stsp@
> diff --git a/sys/dev/sdmmc/sdmmc_mem.c b/sys/dev/sdmmc/sdmmc_mem.c
> index fae8d63912d..715c412e6ea 100644
> --- a/sys/dev/sdmmc/sdmmc_mem.c
> +++ b/sys/dev/sdmmc/sdmmc_mem.c
> @@ -465,8 +465,10 @@ sdmmc_mem_send_scr(struct sdmmc_softc *sc, uint32_t *scr)
> int error = 0;
>
> ptr = malloc(datalen, M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (ptr == NULL)
> + if (ptr == NULL) {
> + error = ENOMEM;
> goto out;
> + }
>
> memset(, 0, sizeof(cmd));
> cmd.c_data = ptr;
>
>
