[PATCH linux dev-4.7 v2] drivers: fsi: Copy all data out

[PATCH linux dev-4.7 v2] drivers: fsi: Copy all data out

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Driver was only copying 32 bits out to the user-provided data pointer.

since v1:
 * don't do a for loop... thanks Milton.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-master-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 3ed82ea..8c8a8f4 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -274,7 +274,7 @@ static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected,
 			resp <<= bits_remaining;
 			resp |= response.msg;
 			bits_received += bits_remaining;
-			*((uint32_t *)data) = response.msg;
+			memcpy(data, &response.msg, size);
 		}
 
 		crc_in = fsi_crc4(0, resp | (0x1ULL << bits_received),
-- 
1.8.3.1

Re: [PATCH linux dev-4.7 v2] drivers: fsi: Copy all data out

[Joel Stanley]

On Tue, Feb 21, 2017 at 9:30 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Driver was only copying 32 bits out to the user-provided data pointer.
>
> since v1:
>  * don't do a for loop... thanks Milton.

Thanks for adding the changelog. Convention is to put it blow the
"--", which means it's not included in the commit log.

>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-master-gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 3ed82ea..8c8a8f4 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -274,7 +274,7 @@ static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected,
>                         resp <<= bits_remaining;
>                         resp |= response.msg;
>                         bits_received += bits_remaining;
> -                       *((uint32_t *)data) = response.msg;
> +                       memcpy(data, &response.msg, size);

This change looks good. I will apply it as-is. However, we have another issue:

When reviewing I took a look at some of the call sites. The driver does this:

        rc = poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);


So we have:

static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected,
                        uint8_t size, void *data)
{
...
        struct fsi_gpio_msg response, cmd;
        int bits_remaining = 0, bit_count, response_id, id;

...
                serial_in(master, &response, bit_count);
                response_id = response.msg & 0x3;
...
                switch (response_id) {
                case FSI_GPIO_RESP_ACK:
                        if (expected == FSI_GPIO_RESP_ACKD)
                                bits_remaining = 8 * size;
                        break;
...
                /* Read in the data field if applicable */
                if (bits_remaining) {
                        serial_in(master, &response, bits_remaining);
                        resp <<= bits_remaining;
                        resp |= response.msg;
                        bits_received += bits_remaining;
                        *((uint32_t *)data) = response.msg;
                }

Data is still null, so we now have a null pointer dereference.

Can you or Chris please take a look at this?

Cheers,

Joel



>                 }
>
>                 crc_in = fsi_crc4(0, resp | (0x1ULL << bits_received),
> --
> 1.8.3.1
>