RE: [RFC PATCH 2/3] powerpc/pseries: Define & use a type for the plpar_hcall() retvals

[David Laight]

From: Balbir Singh
> Sent: 19 October 2016 15:00
...
> Here is an example
> 
> - *slot = retbuf[0];
> + *slot = retvals.v[0];
> 
> Could we hide retvals.v[0] under a macro like
> 
> *slot = hcalls_ret_val(retvals, 0);

Ugg..

> Since we could end up with similar issues if
> someone dereferenced retvals.v[4]

The compiler will detect that these days.

David

Re: [RFC PATCH 2/3] powerpc/pseries: Define & use a type for the plpar_hcall() retvals

[Balbir Singh]

On 19/10/16 22:47, Michael Ellerman wrote:
> Balbir Singh  writes:
> 
>> On 18/10/16 19:40, Michael Ellerman wrote:
>>> We have now had two nasty stack corruption bugs caused by incorrect
>>> sizing of the return buffer for plpar_hcall()/plpar_hcall9().
>>>
>>> To avoid any more such bugs, define a type which encodes the size of the
>>> return buffer, and change the argument of plpar_hcall() to be of that
>>> type, meaning the compiler will check for us that we passed the right
>>> size buffer.
>>>
>>> There isn't an easy way to do this incrementally, without introducing a
>>> new function name, eg. plpar_hcall_with_struct(), which is ugly as hell.
>>> So just do it in one tree-wide change.
>>>
>> Conceptually looks god, but I think we need to abstract the return values
>> as well. I'll test and see if I can send you something on top of this
> 
> Not sure I know what you mean.

Here is an example

-   *slot = retbuf[0];
+   *slot = retvals.v[0];

Could we hide retvals.v[0] under a macro like 

*slot = hcalls_ret_val(retvals, 0);

Since we could end up with similar issues if
someone dereferenced retvals.v[4]

Since we are abstracting under retvals, I was wondering
if we want to further abstract the return values
as well and make retvals opaque to the user


Balbir Singh.

Re: [RFC PATCH 2/3] powerpc/pseries: Define & use a type for the plpar_hcall() retvals

[Michael Ellerman]

Balbir Singh  writes:

> On 18/10/16 19:40, Michael Ellerman wrote:
>> We have now had two nasty stack corruption bugs caused by incorrect
>> sizing of the return buffer for plpar_hcall()/plpar_hcall9().
>> 
>> To avoid any more such bugs, define a type which encodes the size of the
>> return buffer, and change the argument of plpar_hcall() to be of that
>> type, meaning the compiler will check for us that we passed the right
>> size buffer.
>> 
>> There isn't an easy way to do this incrementally, without introducing a
>> new function name, eg. plpar_hcall_with_struct(), which is ugly as hell.
>> So just do it in one tree-wide change.
>> 
> Conceptually looks god, but I think we need to abstract the return values
> as well. I'll test and see if I can send you something on top of this

Not sure I know what you mean.

cheers

Re: [RFC PATCH 2/3] powerpc/pseries: Define & use a type for the plpar_hcall() retvals

[Balbir Singh]

On 18/10/16 19:40, Michael Ellerman wrote:
> We have now had two nasty stack corruption bugs caused by incorrect
> sizing of the return buffer for plpar_hcall()/plpar_hcall9().
> 
> To avoid any more such bugs, define a type which encodes the size of the
> return buffer, and change the argument of plpar_hcall() to be of that
> type, meaning the compiler will check for us that we passed the right
> size buffer.
> 
> There isn't an easy way to do this incrementally, without introducing a
> new function name, eg. plpar_hcall_with_struct(), which is ugly as hell.
> So just do it in one tree-wide change.
> 
Conceptually looks god, but I think we need to abstract the return values
as well. I'll test and see if I can send you something on top of this
Balbir