The raw message frame length is unaligned and explicitly marked as
little endian. It should not be accessed without the appropriatte
accessor functions. Fix this.
Reported-by: kernel-test-robot <[email protected]>
Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index 583315db8b02..9a78188d8d1c 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -1774,7 +1774,8 @@ 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(get_unaligned_le16(&frame->len));
}
static int ssh_ptl_rx_threadfn(void *data)
--
2.30.0
Acked-by: Mark Gross <[email protected]>
On Thu, Feb 11, 2021 at 12:04:11AM +0100, Maximilian Luz wrote:
> The raw message frame length is unaligned and explicitly marked as
> little endian. It should not be accessed without the appropriatte
> accessor functions. Fix this.
>
> Reported-by: kernel-test-robot <[email protected]>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
> drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 583315db8b02..9a78188d8d1c 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1774,7 +1774,8 @@ 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(get_unaligned_le16(&frame->len));
> }
>
> static int ssh_ptl_rx_threadfn(void *data)
> --
> 2.30.0
>
On Thu, Feb 11, 2021 at 12:04:11AM +0100, Maximilian Luz wrote:
> The raw message frame length is unaligned and explicitly marked as
> little endian. It should not be accessed without the appropriatte
> accessor functions. Fix this.
Reviewed-by: Andy Shevchenko <[email protected]>
Though a few nit-picks below.
> Reported-by: kernel-test-robot <[email protected]>
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <[email protected]>
> ---
> drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 583315db8b02..9a78188d8d1c 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1774,7 +1774,8 @@ 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(get_unaligned_le16(&frame->len));
I would leave + on previous line.
Also it's possible to annotate temporary variable and use it, but it seems not
worth to do.
Side question: Do you think the below is correct (& operator)?
sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
To me seems like you take an address to len member rather its value.
> }
>
> static int ssh_ptl_rx_threadfn(void *data)
--
With Best Regards,
Andy Shevchenko
On 2/11/21 11:22 AM, Andy Shevchenko wrote:
> On Thu, Feb 11, 2021 at 12:04:11AM +0100, Maximilian Luz wrote:
>> The raw message frame length is unaligned and explicitly marked as
>> little endian. It should not be accessed without the appropriatte
>> accessor functions. Fix this.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Though a few nit-picks below.
>
>> Reported-by: kernel-test-robot <[email protected]>
>> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
>> Signed-off-by: Maximilian Luz <[email protected]>
>> ---
>> drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
>> index 583315db8b02..9a78188d8d1c 100644
>> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
>> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
>> @@ -1774,7 +1774,8 @@ 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(get_unaligned_le16(&frame->len));
>
> I would leave + on previous line.
I can fix that if it bugs you.
> Also it's possible to annotate temporary variable and use it, but it seems not
> worth to do.
Now that you mention it, we already have the correct frame length in
payload.len. Let me draft up a new patch with that.
> Side question: Do you think the below is correct (& operator)?
>
> sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
>
> To me seems like you take an address to len member rather its value.
That's the point though, no? The signature is
u16 get_unaligned_le16(const void *p)
so we do want a pointer to the len member. So I believe that is correct.
>
>> }
>>
>> static int ssh_ptl_rx_threadfn(void *data)
>
On Thu, Feb 11, 2021 at 12:58:48PM +0100, Maximilian Luz wrote:
> On 2/11/21 11:22 AM, Andy Shevchenko wrote:
> > On Thu, Feb 11, 2021 at 12:04:11AM +0100, Maximilian Luz wrote:
> > > The raw message frame length is unaligned and explicitly marked as
> > > little endian. It should not be accessed without the appropriatte
> > > accessor functions. Fix this.
...
> > Also it's possible to annotate temporary variable and use it, but it seems not
> > worth to do.
>
> Now that you mention it, we already have the correct frame length in
> payload.len. Let me draft up a new patch with that.
Good!
> > Side question: Do you think the below is correct (& operator)?
> >
> > sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
> >
> > To me seems like you take an address to len member rather its value.
>
> That's the point though, no? The signature is
>
> u16 get_unaligned_le16(const void *p)
>
> so we do want a pointer to the len member. So I believe that is correct.
Indeed. I messed up with le16_to_cpu().
--
With Best Regards,
Andy Shevchenko