2021-02-11 13:10:29

by Maximilian Luz

[permalink] [raw]
Subject: [PATCH v2] platform/surface: aggregator: Fix access of unaligned value

The raw message frame length is unaligned and explicitly marked as
little endian. It should not be accessed without the appropriate
accessor functions. Fix this.

Note that payload.len already contains the correct length after parsing
via sshp_parse_frame(), so we can simply use that instead.

Reported-by: kernel-test-robot <[email protected]>
Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <[email protected]>
---

Changes in v2:
- Use payload.len instead of getting the frame length directly. Note
that payload.len equals the frame length and is already correctly set
in sshp_parse_frame(), so they are exactly the same thing. Makes it
look a bit nicer though.

I did drop the ACKs/Reveiewd-by in case you want to check that
yourselves and since that's essentially the whole change.

---
drivers/platform/surface/aggregator/ssh_packet_layer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index 583315db8b02..15d96eac6811 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -1774,7 +1774,7 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
break;
}

- return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len);
+ return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(payload.len);
}

static int ssh_ptl_rx_threadfn(void *data)
--
2.30.0


2021-02-11 13:34:08

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2] platform/surface: aggregator: Fix access of unaligned value

On Thu, Feb 11, 2021 at 01:41:49PM +0100, Maximilian Luz wrote:
> The raw message frame length is unaligned and explicitly marked as
> little endian. It should not be accessed without the appropriate
> accessor functions. Fix this.
>
> Note that payload.len already contains the correct length after parsing
> via sshp_parse_frame(), so we can simply use that instead.

Reviewed-by: Andy Shevchenko <[email protected]>

> Reported-by: kernel-test-robot <[email protected]>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
>
> Changes in v2:
> - Use payload.len instead of getting the frame length directly. Note
> that payload.len equals the frame length and is already correctly set
> in sshp_parse_frame(), so they are exactly the same thing. Makes it
> look a bit nicer though.
>
> I did drop the ACKs/Reveiewd-by in case you want to check that
> yourselves and since that's essentially the whole change.
>
> ---
> drivers/platform/surface/aggregator/ssh_packet_layer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 583315db8b02..15d96eac6811 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1774,7 +1774,7 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
> break;
> }
>
> - return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len);
> + return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(payload.len);
> }
>
> static int ssh_ptl_rx_threadfn(void *data)
> --
> 2.30.0
>

--
With Best Regards,
Andy Shevchenko


2021-02-11 17:01:39

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v2] platform/surface: aggregator: Fix access of unaligned value

Hi,

On 2/11/21 1:41 PM, Maximilian Luz wrote:
> The raw message frame length is unaligned and explicitly marked as
> little endian. It should not be accessed without the appropriate
> accessor functions. Fix this.
>
> Note that payload.len already contains the correct length after parsing
> via sshp_parse_frame(), so we can simply use that instead.
>
> Reported-by: kernel-test-robot <[email protected]>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <[email protected]>

Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
>
> Changes in v2:
> - Use payload.len instead of getting the frame length directly. Note
> that payload.len equals the frame length and is already correctly set
> in sshp_parse_frame(), so they are exactly the same thing. Makes it
> look a bit nicer though.
>
> I did drop the ACKs/Reveiewd-by in case you want to check that
> yourselves and since that's essentially the whole change.
>
> ---
> drivers/platform/surface/aggregator/ssh_packet_layer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 583315db8b02..15d96eac6811 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1774,7 +1774,7 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source)
> break;
> }
>
> - return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len);
> + return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(payload.len);
> }
>
> static int ssh_ptl_rx_threadfn(void *data)
>