* [PATCH] mmc: tmio-mmc: fix bad pointer math
@ 2017-08-01 20:43 Chris Brandt
2017-08-01 21:03 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: Chris Brandt @ 2017-08-01 20:43 UTC (permalink / raw)
To: stable; +Cc: Geert Uytterhoeven, Wolfram Sang, Ulf Hansson, Chris Brandt
commit 9c284c41c0886f09e75c323a16278b6d353b0b4a upstream.
The existing code gives an incorrect pointer value.
The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
number in bytes. A cast of buf should have been used.
However, instead of casting, just change the code to use u32 pointers.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/host/tmio_mmc_pio.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index a2d92f10501b..a3d20e39e5b5 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -404,30 +404,29 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
* Transfer the data
*/
if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
- u8 data[4] = { };
+ u32 data = 0;
+ u32 *buf32 = (u32 *)buf;
if (is_read)
- sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
+ sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);
else
- sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
+ sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);
/* if count was multiple of 4 */
if (!(count & 0x3))
return;
- buf8 = (u8 *)(buf + (count >> 2));
+ buf32 += count >> 2;
count %= 4;
if (is_read) {
- sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
- (u32 *)data, 1);
- memcpy(buf8, data, count);
+ sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1);
+ memcpy(buf32, &data, count);
} else {
- memcpy(data, buf8, count);
- sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
- (u32 *)data, 1);
+ memcpy(&data, buf32, count);
+ sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1);
}
return;
--
2.13.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-08-01 20:43 [PATCH] mmc: tmio-mmc: fix bad pointer math Chris Brandt
@ 2017-08-01 21:03 ` Greg KH
2017-08-02 12:08 ` Chris Brandt
0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2017-08-01 21:03 UTC (permalink / raw)
To: Chris Brandt; +Cc: stable, Geert Uytterhoeven, Wolfram Sang, Ulf Hansson
On Tue, Aug 01, 2017 at 03:43:34PM -0500, Chris Brandt wrote:
> commit 9c284c41c0886f09e75c323a16278b6d353b0b4a upstream.
>
> The existing code gives an incorrect pointer value.
> The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
> number in bytes. A cast of buf should have been used.
>
> However, instead of casting, just change the code to use u32 pointers.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/host/tmio_mmc_pio.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
What kernel tree(s) is this for?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-08-01 21:03 ` Greg KH
@ 2017-08-02 12:08 ` Chris Brandt
2017-08-02 13:51 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: Chris Brandt @ 2017-08-02 12:08 UTC (permalink / raw)
To: Greg KH; +Cc: stable, Geert Uytterhoeven, Wolfram Sang, Ulf Hansson
On Tuesday, August 01, 2017, Greg KH wrote:
> On Tue, Aug 01, 2017 at 03:43:34PM -0500, Chris Brandt wrote:
> > commit 9c284c41c0886f09e75c323a16278b6d353b0b4a upstream.
> >
> > The existing code gives an incorrect pointer value.
> > The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
> > number in bytes. A cast of buf should have been used.
> >
> > However, instead of casting, just change the code to use u32 pointers.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/mmc/host/tmio_mmc_pio.c | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
>
> What kernel tree(s) is this for?
>
> thanks,
>
> greg k-h
I created this patch using the current 4.12.y branch. The code it fixes
went in 4.10.
The one thing I couldn't figure out by just looking at other stable
commits was how you were supposed to know what tree I based the patch off so
you had an idea where/how to apply it to. All I saw was that people
just added 'commit xxxx upstream' to the commit log, but not what it was
backported to.
How was I supposed to give that info to you?
Thank you,
Chris
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-08-02 12:08 ` Chris Brandt
@ 2017-08-02 13:51 ` Greg KH
0 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2017-08-02 13:51 UTC (permalink / raw)
To: Chris Brandt; +Cc: stable, Geert Uytterhoeven, Wolfram Sang, Ulf Hansson
On Wed, Aug 02, 2017 at 12:08:39PM +0000, Chris Brandt wrote:
> On Tuesday, August 01, 2017, Greg KH wrote:
> > On Tue, Aug 01, 2017 at 03:43:34PM -0500, Chris Brandt wrote:
> > > commit 9c284c41c0886f09e75c323a16278b6d353b0b4a upstream.
> > >
> > > The existing code gives an incorrect pointer value.
> > > The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
> > > number in bytes. A cast of buf should have been used.
> > >
> > > However, instead of casting, just change the code to use u32 pointers.
> > >
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
> > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > > drivers/mmc/host/tmio_mmc_pio.c | 19 +++++++++----------
> > > 1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > What kernel tree(s) is this for?
> >
> > thanks,
> >
> > greg k-h
>
> I created this patch using the current 4.12.y branch. The code it fixes
> went in 4.10.
>
> The one thing I couldn't figure out by just looking at other stable
> commits was how you were supposed to know what tree I based the patch off so
> you had an idea where/how to apply it to. All I saw was that people
> just added 'commit xxxx upstream' to the commit log, but not what it was
> backported to.
>
> How was I supposed to give that info to you?
Just say below the --- line, "This patch is for kernel trees X, Y, and
Z" or some such thing.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-07-11 19:37 ` Chris Brandt
@ 2017-07-12 6:50 ` Geert Uytterhoeven
0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2017-07-12 6:50 UTC (permalink / raw)
To: Chris Brandt
Cc: Ulf Hansson, Wolfram Sang, Dan Carpenter, Simon Horman,
Linux MMC List, Linux-Renesas
Hi Chris,
On Tue, Jul 11, 2017 at 9:37 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, July 11, 2017, Geert Uytterhoeven wrote:
>> > zeroing out the bottom 2 bits of count for out math.
>>
>> s/out/our/
>
> Thank you!
>
>> > - buf8 = (u8 *)(buf + (count >> 2));
>> > + buf8 = (u8 *)buf + (count & ~3);
>> > count %= 4;
>>
>> While correct, this is IMHO still difficult to understand for the casual
>> reader.
>>
>> Given the code before casts to "u32 *", and uses "count >>2", and the code
>> after also casts to "u32 *", what about getting rid of all casts like:
>>
>> u32 data = 0;
>> u32 *buf32 = buf;
>>
>> if (is_read)
>> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32,
>> count >> 2);
>> else
>> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32,
>> count >> 2);
>>
>> /* if count was multiple of 4 */
>> if (!(count & 0x3))
>> return;
>>
>> buf32 += count >> 2;
>> count %= 4;
>>
>> if (is_read) {
>> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1);
>> memcpy(buf32, &data, count);
>> } else {
>> memcpy(&data, buf32, count);
>> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1);
>> }
>>
>> return;
>> }
>>
>
> Good idea. I just tried it and it seems to work. I'll resend a patch.
>
>
>> u32 *buf32 = buf;
>
> GCC didn't like this line without casting buf to a u32 *. It threw an
> error, not just a warning. Go figure.
Sorry, the cast is indeed missing, as buf is not a void *.
> Question:
>> u32 data = 0;
>
> Any special reason why you are initializing this to 0????
I think the original code did that, too. Hmm, I got mislead by the curly braces,
there's no "0" in between them.
It's also a bit safer to not write uninitialized data to the CTL_SD_DATA_PORT
register.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-07-11 17:55 ` Geert Uytterhoeven
@ 2017-07-11 19:37 ` Chris Brandt
2017-07-12 6:50 ` Geert Uytterhoeven
0 siblings, 1 reply; 8+ messages in thread
From: Chris Brandt @ 2017-07-11 19:37 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Ulf Hansson, Wolfram Sang, Dan Carpenter, Simon Horman,
Linux MMC List, Linux-Renesas
Hi Geert,
On Tuesday, July 11, 2017, Geert Uytterhoeven wrote:
> > zeroing out the bottom 2 bits of count for out math.
>
> s/out/our/
Thank you!
> > - buf8 = (u8 *)(buf + (count >> 2));
> > + buf8 = (u8 *)buf + (count & ~3);
> > count %= 4;
>
> While correct, this is IMHO still difficult to understand for the casual
> reader.
>
> Given the code before casts to "u32 *", and uses "count >>2", and the code
> after also casts to "u32 *", what about getting rid of all casts like:
>
> u32 data = 0;
> u32 *buf32 = buf;
>
> if (is_read)
> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32,
> count >> 2);
> else
> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32,
> count >> 2);
>
> /* if count was multiple of 4 */
> if (!(count & 0x3))
> return;
>
> buf32 += count >> 2;
> count %= 4;
>
> if (is_read) {
> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1);
> memcpy(buf32, &data, count);
> } else {
> memcpy(&data, buf32, count);
> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1);
> }
>
> return;
> }
>
Good idea. I just tried it and it seems to work. I'll resend a patch.
> u32 *buf32 = buf;
GCC didn't like this line without casting buf to a u32 *. It threw an
error, not just a warning. Go figure.
Question:
> u32 data = 0;
Any special reason why you are initializing this to 0????
Thank you,
Chris
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: tmio-mmc: fix bad pointer math
2017-07-11 16:29 Chris Brandt
@ 2017-07-11 17:55 ` Geert Uytterhoeven
2017-07-11 19:37 ` Chris Brandt
0 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2017-07-11 17:55 UTC (permalink / raw)
To: Chris Brandt
Cc: Ulf Hansson, Wolfram Sang, Dan Carpenter, Simon Horman,
Linux MMC List, Linux-Renesas
Hi Chris,
On Tue, Jul 11, 2017 at 6:29 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> The existing code gives an incorrect value.
> The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
> number in bytes, so the pointer should have been cast before doing any
> pointer arithmetic.
>
> Since we know the code before it is doing as many 4-byte transfers as
> possible, we just need a pointer to where it left off in the buffer, hence
> zeroing out the bottom 2 bits of count for out math.
s/out/our/
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> drivers/mmc/host/tmio_mmc_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 77e7b56a9099..5dfc556ccedf 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -428,7 +428,7 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
> if (!(count & 0x3))
> return;
>
> - buf8 = (u8 *)(buf + (count >> 2));
> + buf8 = (u8 *)buf + (count & ~3);
> count %= 4;
While correct, this is IMHO still difficult to understand for the casual reader.
Given the code before casts to "u32 *", and uses "count >>2", and the code
after also casts to "u32 *", what about getting rid of all casts like:
u32 data = 0;
u32 *buf32 = buf;
if (is_read)
sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);
else
sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);
/* if count was multiple of 4 */
if (!(count & 0x3))
return;
buf32 += count >> 2;
count %= 4;
if (is_read) {
sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1);
memcpy(buf32, &data, count);
} else {
memcpy(&data, buf32, count);
sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1);
}
return;
}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mmc: tmio-mmc: fix bad pointer math
@ 2017-07-11 16:29 Chris Brandt
2017-07-11 17:55 ` Geert Uytterhoeven
0 siblings, 1 reply; 8+ messages in thread
From: Chris Brandt @ 2017-07-11 16:29 UTC (permalink / raw)
To: Ulf Hansson, Wolfram Sang
Cc: Dan Carpenter, Simon Horman, linux-mmc, linux-renesas-soc, Chris Brandt
The existing code gives an incorrect value.
The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
number in bytes, so the pointer should have been cast before doing any
pointer arithmetic.
Since we know the code before it is doing as many 4-byte transfers as
possible, we just need a pointer to where it left off in the buffer, hence
zeroing out the bottom 2 bits of count for out math.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port")
Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
drivers/mmc/host/tmio_mmc_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 77e7b56a9099..5dfc556ccedf 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -428,7 +428,7 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
if (!(count & 0x3))
return;
- buf8 = (u8 *)(buf + (count >> 2));
+ buf8 = (u8 *)buf + (count & ~3);
count %= 4;
if (is_read) {
--
2.13.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-02 13:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-01 20:43 [PATCH] mmc: tmio-mmc: fix bad pointer math Chris Brandt
2017-08-01 21:03 ` Greg KH
2017-08-02 12:08 ` Chris Brandt
2017-08-02 13:51 ` Greg KH
-- strict thread matches above, loose matches on Subject: below --
2017-07-11 16:29 Chris Brandt
2017-07-11 17:55 ` Geert Uytterhoeven
2017-07-11 19:37 ` Chris Brandt
2017-07-12 6:50 ` Geert Uytterhoeven
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.