Differential D38739
mprutil: "fix user reply buffer (64)..." warnings Authored by asomers on Feb 22 2023, 10:11 PM. Tags None Referenced Files
Details
Depending on the card's firmware version, it may return different length PR: 264848
Diff Detail
Event TimelineComment Actions Looking at mpr_user_pass_thru, it does: if (sz > data->ReplySize /* factslen */) {
print-a-warning
}
copyout(cm->cm_reply, PTRIN(data->PtrReply), data->ReplySize);Now we can have a situation where we're copying out more data than the controller returned, i.e., data->ReplySize > sz. Do mps and mpr handle this case correctly? Comment Actions the userland part of this looks good. It doesn't hurt to allocate too much here, since the older cards will just not use the space. Comment Actions data->ReplySize is the size of the user buffer, so we're never copying out more data than that. sz is the number of bytes the card returned in its reply. The reply is memory that's card managed. We get the correct size inside of mps or mpr based on definitions in the mpi/mpi2_ioc.h file for the MPI2_IOC_FACTS_REPLY buffer. The real problem here is that we include mps before mpr, and so get the mps size for this rather than the mpr size because the mpi2_ioc.h files are both protected by the same guard include define. mpr's /* IOCFacts Reply message */
typedef struct _MPI2_IOC_FACTS_REPLY
{
U16 MsgVersion; /* 0x00 */
U8 MsgLength; /* 0x02 */
[trimmed]
U16 MinDevHandle; /* 0x3C */
U8 CurrentHostPageSize; /* 0x3E */
U8 Reserved4; /* 0x3F */
U8 SGEModifierMask; /* 0x40 */
U8 SGEModifierValue; /* 0x41 */
U8 SGEModifierShift; /* 0x42 */
U8 Reserved5; /* 0x43 */
} MPI2_IOC_FACTS_REPLY, MPI2_POINTER PTR_MPI2_IOC_FACTS_REPLY,mps' /* IOCFacts Reply message */
typedef struct _MPI2_IOC_FACTS_REPLY
{
U16 MsgVersion; /* 0x00 */
U8 MsgLength; /* 0x02 */
[trimmed]
U16 MinDevHandle; /* 0x3C */
U16 Reserved4; /* 0x3E */
} MPI2_IOC_FACTS_REPLY, MPI2_POINTER PTR_MPI2_IOC_FACTS_REPLY,Comment Actions Good find, @imp . I think this means we need to split mps_cmd into separate source files for mps and mpr. Comment Actions The following also works. There are 2.5 and 2.6 versions of the protocol. I just have docs on 2.0 and 2.6, diff --git a/usr.sbin/mpsutil/mps_cmd.c b/usr.sbin/mpsutil/mps_cmd.c index a9cb269abc5f..6f484913d848 100644 --- a/usr.sbin/mpsutil/mps_cmd.c +++ b/usr.sbin/mpsutil/mps_cmd.c @@ -739,7 +739,7 @@ mps_get_iocfacts(int fd) len = sizeof(msgver); error = sysctlbyname(sysctlname, msgver, &len, NULL, 0); if (error == 0) { - if (strncmp(msgver, "2.6", sizeof(msgver)) == 0) + if (strcmp(msgver, "2.5") >= 0) factslen += 4; } The number of structures that are a different size, but the same name, between MPI2 and MPI2.6. There's + * NOTE: Names (typedefs, defines, etc.) beginning with an MPI25 or Mpi25 + * prefix are for use only on MPI v2.5 products, and must not be used + * with MPI v2.0 products. Unless otherwise noted, names beginning with + * MPI2 or Mpi2 are for use with both MPI v2.0 and MPI v2.5 products. But it looks like they aren't super interchangeable for other reasons: One alternative is to use the same mpi2 files, from mpr. The mpi2 files Another alternative is to include mpr first. That should have the same Having two different versions for mpr and mps seems silly: This is Just adding 4 and having a good comment is likely all that's needed here.
Comment Actions Please don't commit this. The whole point of the sysctl is to find out if you need to adjust the size of the buffer, and to provide an easy way to update the code as new versions of the MPI spec came out over time. At the time this code was written, 2.6 was the highest version of the spec, so that's all that was implemented here. But what if 2.7 came out and required +8? Or -4? Or some other variant? The intention was to leave open the door for other adjustments that might be required for versions later than 2.6, and my error was not providing a comment in the code, though this was hinted at in the commit message history. So the real problem is likely that the msgver is something numerically higher than 2.6, and the code needs to be updated to account for it. Please don't nerf it like you're trying to do in this patch. Comment Actions The root cause here is that a "Avago Technologies (LSI) SAS3008" and possibly other cards are returning "2.5" rather than "2.6" for the msg_version, but the 2.5 version requires 68 bytes instead of 64 bytes, so the strcmp test I wrote would work for that. It fixes the issue nicely for my SAS3008 I have, for example. My "Broadcom Inc. (LSI) HARD SEC SAS3816" has 2.6 msg format. It's unclear what's next for the MPI 2.x line: Broadcom's newest cards are Fusion MPT 3.0, whose data structures are incompatible with the MPI 2.x cards. Comment Actions Ok, if the 2.5 msgver attribute is requiring 64bytes in some cases but 68bytes in other cases, that sucks, and it's a blatant violation of the 2.5 spec. Really, the motivation here was that any time a new msgver came out, investigation and review would be required to verify the sizes of the messages, instead of just blindly assuming sizes, but I forgot to put an assert into the CLI for that. I think that this would also impact the driver. I wonder if it’s possible to ask for just the first DWORD of the message and see what the firmware returns back as the residual, then ask for the rest based on that return value. No matter what, just assuming +4 is creating technical debt that I advise against. Comment Actions @scottl as discussed in bugzilla, the actual msgver is 2.5, at least in my case. So the original commit wrongly guessed which versions require the larger struct. Also, what's so bad about unconditionally allocating an extra 4 bytes? It's cheaper than checking the sysctl.
Comment Actions typedef struct _MPI2_IOC_FACTS_REPLY
{
U16 MsgVersion; /* 0x00 */
U8 MsgLength; /* 0x02 */
U8 Function; /* 0x03 */So yeah, just ask for 4 bytes, get the MsgLength, then ask for the rest based on that. No more magic numbers needed when you do that. If you decide to take out the sysctl code in the code block, then there's no more reason for the sysctl in the kernel, so it can probably be deprecated. Comment Actions When I try this on an mps card, I get MsgLength=16. Does that mean that all fields past the 16th byte are garbage? I'll try this on an mpr card after lunch. Comment Actions The MsgLength field is in terms of DWORDs, i.e. 32bit words. A value of 16 means 64 bytes. Comment Actions One problem with asking for the length, though, is that it will still trigger the warning in the kernel since 4 or 8 would be < the 64 or 68 bytes that is returned by the card, unless the driver tracks this when it does its IOC_FACTS and publishes that size as sysctl (which is likely overkill). Perhaps the warning should just be under bootverbose with probing the length here. Other users like lsiutil still use the old, smaller buffer, but since nothing looks at the extra bytes and there's no buffer overflow to worry about, I think that's safe to move to bootverbose.
Comment Actions An mpr card will never be < 2.5, since the driver was created for 2.5 and higher cards. The msg format version is returned in first two bytes and the drivers publishes that as this sysctl. and mpr won't be used for cards > 2.6, since the next version is 3.0, and all the structures are laid out differently with different sizes in that protocol version. There's a whole new driver for the new 3.0 cards. Given the supply chain shortages that forced a fast shift to the 3.0 cards, I highly doubt there will ever be 2.7 cards, but it's easy enough to get the length like Scott suggests and use that in case that prediction is wrong. Comment Actions
Comment Actions Looks like this review got away from me. Oops. I'll rebase, test, and hopefully commit today. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||