Packet Size Mismatches

Packet Size Mismatches

[Patrick Venture]

Per the maximum response size patch
(https://gerrit.openbmc-project.xyz/15743), the goal is to know how
many bytes can be returned by the channel.  This can vary by channel,
per the comments, and other related changes.  However, presently,
we're seeing a memory corruption over kcs which has a limit of 256.
So, we're copying way too much data onto the response.  The code
itself has no idea how many bytes it can send.

https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.cpp#L409
is a 64-byte buffer, but the ipmi message could ask for data beyond
that, and leads to a blind memcpy situation.

If I'm reading that patchset (not yet merged) correctly
(https://gerrit.openbmc-project.xyz/16285) it lets the IPMI client
request channel information to know the maximum it can request.
However, what lets the code running ask that question?  How does my
library know it's responding to a legal request?  Or is the idea that
with the reworking that's underway it'll only be able to provide legal
responses automatically for the channel on which the request came?

(Also, unrelated, but at some point is the goal still to have one
daemon handle all ipmi traffic, including network ipmi traffic, via a
bridge similar to kcsbridge and btbridge?)

Patrick

Re: Packet Size Mismatches

[Patrick Venture]

On Wed, Jan 9, 2019 at 9:10 AM Patrick Venture <venture@google.com> wrote:
>
> Per the maximum response size patch
> (https://gerrit.openbmc-project.xyz/15743), the goal is to know how
> many bytes can be returned by the channel.  This can vary by channel,
> per the comments, and other related changes.  However, presently,
> we're seeing a memory corruption over kcs which has a limit of 256.
> So, we're copying way too much data onto the response.  The code
> itself has no idea how many bytes it can send.
>
> https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.cpp#L409
> is a 64-byte buffer, but the ipmi message could ask for data beyond
> that, and leads to a blind memcpy situation.
>
> If I'm reading that patchset (not yet merged) correctly
> (https://gerrit.openbmc-project.xyz/16285) it lets the IPMI client
> request channel information to know the maximum it can request.
> However, what lets the code running ask that question?  How does my
> library know it's responding to a legal request?  Or is the idea that
> with the reworking that's underway it'll only be able to provide legal
> responses automatically for the channel on which the request came?

It looks like that method is in an installed header, and should be
accessible from downstream callers, however, how does a downstream
caller know the channel through which their request came?

>
> (Also, unrelated, but at some point is the goal still to have one
> daemon handle all ipmi traffic, including network ipmi traffic, via a
> bridge similar to kcsbridge and btbridge?)
>
> Patrick

Re: Packet Size Mismatches

[Vernon Mauery]

On 09-Jan-2019 09:10 AM, Patrick Venture wrote:
>Per the maximum response size patch
>(https://gerrit.openbmc-project.xyz/15743), the goal is to know how
>many bytes can be returned by the channel.  This can vary by channel,
>per the comments, and other related changes.  However, presently,
>we're seeing a memory corruption over kcs which has a limit of 256.
>So, we're copying way too much data onto the response.  The code
>itself has no idea how many bytes it can send.

This is a tricky problem.... Ideally, if an IPMI command wants to return 
a large number of bytes, it would consult its context and return an 
error if the response would not fit in the channel. This relies on each 
command to be written correctly. In a slightly less ideal world, the 
ipmid daemon would just return the bytes that the handler returned and 
the bridge (kcs, bt, rmcp+, etc.) would then refuse to package up a 
response that is larger than the max payload size and return an error 
instead. The problem with this is that information is lost.

>https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.cpp#L409
>is a 64-byte buffer, but the ipmi message could ask for data beyond
>that, and leads to a blind memcpy situation.
>
>If I'm reading that patchset (not yet merged) correctly
>(https://gerrit.openbmc-project.xyz/16285) it lets the IPMI client
>request channel information to know the maximum it can request.
>However, what lets the code running ask that question?  How does my
>library know it's responding to a legal request?  Or is the idea that
>with the reworking that's underway it'll only be able to provide legal
>responses automatically for the channel on which the request came?

I have a patch in review (it actually has a +2, but it is waiting on a 
parent), that exposes the channel size to the IPMI stack as it is 
running. This allows an IPMI command to query the channel size.

https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-host-ipmid/+/16285

>(Also, unrelated, but at some point is the goal still to have one
>daemon handle all ipmi traffic, including network ipmi traffic, via a
>bridge similar to kcsbridge and btbridge?)

Yes. That is exactly what the sum of all my patches against host-ipmi 
and net-ipmi do.

--Vernon

Re: Packet Size Mismatches

[Patrick Venture]

On Thu, Jan 10, 2019 at 2:06 PM Vernon Mauery
<vernon.mauery@linux.intel.com> wrote:
>
> On 09-Jan-2019 09:10 AM, Patrick Venture wrote:
> >Per the maximum response size patch
> >(https://gerrit.openbmc-project.xyz/15743), the goal is to know how
> >many bytes can be returned by the channel.  This can vary by channel,
> >per the comments, and other related changes.  However, presently,
> >we're seeing a memory corruption over kcs which has a limit of 256.
> >So, we're copying way too much data onto the response.  The code
> >itself has no idea how many bytes it can send.
>
> This is a tricky problem.... Ideally, if an IPMI command wants to return
> a large number of bytes, it would consult its context and return an
> error if the response would not fit in the channel. This relies on each
> command to be written correctly. In a slightly less ideal world, the

Obviously the 64 byte response buffer in impid will be fixed at some
point shortly, probably in your currently staged patches.

Is the context something new being passed to handlers?  (I haven't had
a chance to really dig through your full stack of patches).

> ipmid daemon would just return the bytes that the handler returned and
> the bridge (kcs, bt, rmcp+, etc.) would then refuse to package up a
> response that is larger than the max payload size and return an error
> instead. The problem with this is that information is lost.
>
> >https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.cpp#L409
> >is a 64-byte buffer, but the ipmi message could ask for data beyond
> >that, and leads to a blind memcpy situation.
> >
> >If I'm reading that patchset (not yet merged) correctly
> >(https://gerrit.openbmc-project.xyz/16285) it lets the IPMI client
> >request channel information to know the maximum it can request.
> >However, what lets the code running ask that question?  How does my
> >library know it's responding to a legal request?  Or is the idea that
> >with the reworking that's underway it'll only be able to provide legal
> >responses automatically for the channel on which the request came?
>
> I have a patch in review (it actually has a +2, but it is waiting on a
> parent), that exposes the channel size to the IPMI stack as it is
> running. This allows an IPMI command to query the channel size.
>
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-host-ipmid/+/16285

Good.

>
> >(Also, unrelated, but at some point is the goal still to have one
> >daemon handle all ipmi traffic, including network ipmi traffic, via a
> >bridge similar to kcsbridge and btbridge?)
>
> Yes. That is exactly what the sum of all my patches against host-ipmi
> and net-ipmi do.

Great! :D
>
> --Vernon

Re: Packet Size Mismatches

[Vernon Mauery]

On 10-Jan-2019 03:46 PM, Patrick Venture wrote:
>On Thu, Jan 10, 2019 at 2:06 PM Vernon Mauery
><vernon.mauery@linux.intel.com> wrote:
>>
>> On 09-Jan-2019 09:10 AM, Patrick Venture wrote:
>> >Per the maximum response size patch
>> >(https://gerrit.openbmc-project.xyz/15743), the goal is to know how
>> >many bytes can be returned by the channel.  This can vary by channel,
>> >per the comments, and other related changes.  However, presently,
>> >we're seeing a memory corruption over kcs which has a limit of 256.
>> >So, we're copying way too much data onto the response.  The code
>> >itself has no idea how many bytes it can send.
>>
>> This is a tricky problem.... Ideally, if an IPMI command wants to return
>> a large number of bytes, it would consult its context and return an
>> error if the response would not fit in the channel. This relies on each
>> command to be written correctly. In a slightly less ideal world, the
>
>Obviously the 64 byte response buffer in impid will be fixed at some
>point shortly, probably in your currently staged patches.

I am not sure that this particular issue is addressed yet, but the 
patches I have lay a framework that allows it to be more easily fixed.

>Is the context something new being passed to handlers?  (I haven't had
>a chance to really dig through your full stack of patches).

The context is something that can be requested as part of the handler 
signature. The handler can have the request object, the context object, 
or the yield context object. The request object contains all the info, 
the context contains some of the info, and the yield context is only for 
async operations. 

handler.hpp contains the details of what sorts of goodies that can be 
requested on top of ipmi data types.
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-host-ipmid/+/11273/17/ipmi/handler.hpp

--Vernon