Error check string parsing

[ghost]

 > 
 >  phdr = (struct spar_controlvm_parameters_header *)(ctx->data);
 >  switch (which_string) {
 >  case PARSERSTRING_INITIATOR:
 >      ctx->curr = ctx->data + phdr->initiator_offset;
 >      ctx->bytes_remaining = phdr->initiator_length;
 >      break;
 >  case PARSERSTRING_TARGET:
 >      ctx->curr = ctx->data + phdr->target_offset;
 >      ctx->bytes_remaining = phdr->target_length;
 >      break;
 >  case PARSERSTRING_CONNECTION:
 >      ctx->curr = ctx->data + phdr->connection_offset;
 >      ctx->bytes_remaining = phdr->connection_length;
 >      break;
 >  case PARSERSTRING_NAME:
 >      ctx->curr = ctx->data + phdr->name_offset;
 >      ctx->bytes_remaining = phdr->name_length;
 >      break;

Someone is validating that all of those values are "acceptable", right?

If so, where? I missed it. No overflows? No underflows? All correct lengths? Seems really trusting...

[davidker]

Careful with this one. Bryan Thompson made changes that I've put into upstream-next. Might want to review those before you do this one. I'm now wondering if we should have a reported-by from Greg, my guess is NO for this one.

[davidker]

848df4c65b708031130372d3908f872dbe6abd37 and fece603837ff199d59aab92469797ee9d41b9585

[ghost]

The area directly relevant to this issue has shrunk considerably. As of how upstream-next looks today, this is all that remains of the code originally cited in Greg's feedback:

 396 static void *
 397 parser_name_get(struct parser_context *ctx)
 398 {
 399         struct spar_controlvm_parameters_header *phdr = NULL;
 400
 401         phdr = (struct spar_controlvm_parameters_header *)(ctx->data);
 402         ctx->curr = ctx->data + phdr->name_offset;
 403         ctx->bytes_remaining = phdr->name_length;
 404         return parser_string_get(ctx);
 405 }

[davidker]

So the question remains, are we still checking them.

[ghost]

The only locations in the driver set where "potentially unpredictable" strings can come from are controlvm entries and sysfs entries. Since the region of code Greg commented on is the only controlvm entry of interest, we must only look for sysfs entries. It is these regions where I will search next.

[ghost]

The toolaction functions look safe:

$ grep -rn "toolaction" drivers/staging/unisys/
drivers/staging/unisys/visorbus/visorchipset.c:182:static ssize_t toolaction_show(struct device *dev,
drivers/staging/unisys/visorbus/visorchipset.c:194:static ssize_t toolaction_store(struct device *dev,
drivers/staging/unisys/visorbus/visorchipset.c:214:static DEVICE_ATTR_RW(toolaction);
drivers/staging/unisys/visorbus/visorchipset.c:1312:    &dev_attr_toolaction.attr,
drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset:39:                the toolaction field to determine what operation is being
drivers/staging/unisys/Documentation/ABI/sysfs-platform-visorchipset:43:What:           install/toolaction

 182 static ssize_t toolaction_show(struct device *dev,
 183                                struct device_attribute *attr,
 184                                char *buf)
 185 {
 186         u8 tool_action = 0;
 187
 188         visorchannel_read(controlvm_channel,
 189                           offsetof(struct spar_controlvm_channel_protocol,
 190                                    tool_action), &tool_action, sizeof(u8));
 191         return sprintf(buf, "%u\n", tool_action);
 192 }
 193
 194 static ssize_t toolaction_store(struct device *dev,
 195                                 struct device_attribute *attr,
 196                                 const char *buf, size_t count)
 197 {
 198         u8 tool_action;
 199         int ret;
 200
 201         if (kstrtou8(buf, 10, &tool_action))
 202                 return -EINVAL;
 203
 204         ret = visorchannel_write
 205                 (controlvm_channel,
 206                  offsetof(struct spar_controlvm_channel_protocol,
 207                           tool_action),
 208                  &tool_action, sizeof(u8));
 209
 210         if (ret)
 211                 return ret;
 212         return count;
 213 }

We expect and are assured of only reading/writing a byte.

[ghost]

The copy_[to|from]_user functions look safe too:

$ grep -rn "c.*py.*us.*r" drivers/staging/unisys/
drivers/staging/unisys/visorbus/visorchipset.c:1754:            if (copy_to_user((void __user *)arg, &vrtc_offset,
drivers/staging/unisys/visorbus/visorchipset.c:1760:            if (copy_from_user(&adjustment, (void __user *)arg,

1744 static long visorchipset_ioctl(struct file *file, unsigned int cmd,
1745                                unsigned long arg)
1746 {
1747         u64 adjustment;
1748         s64 vrtc_offset;
1749
1750         switch (cmd) {
1751         case VMCALL_QUERY_GUEST_VIRTUAL_TIME_OFFSET:
1752                 /* get the physical rtc offset */
1753                 vrtc_offset = issue_vmcall_query_guest_virtual_time_offset();
1754                 if (copy_to_user((void __user *)arg, &vrtc_offset,
1755                                  sizeof(vrtc_offset))) {
1756                         return -EFAULT;
1757                 }
1758                 return 0;
1759         case VMCALL_UPDATE_PHYSICAL_TIME:
1760                 if (copy_from_user(&adjustment, (void __user *)arg,
1761                                    sizeof(adjustment))) {
1762                         return -EFAULT;
1763                 }
1764                 return issue_vmcall_update_physical_time(adjustment);
1765         default:
1766                 return -EFAULT;
1767         }
1768 }

We expect and are assured of only reading/writing 8 bytes.

[ghost]

The the curr member of the parser_context struct is only used in these locations in visorchipset.c:

374:     pscan = ctx->curr;
402:     ctx->curr = ctx->data + phdr->name_offset;
1842:    ctx->curr = NULL;

The bytes_remaining member is only used in these locations:

403:    ctx->bytes_remaining = phdr->name_length;
1843:   ctx->bytes_remaining = 0;

[ghost]

Branch: githubissue-118-staging-next-TurningWrenches Commit(s): 8e8a665225b3ba390dcf7b25400dd9f020c4b238 checkpatch-cleared: Yes T710-1 verified (smoke-test): Yes

[davidker]

Committed to staging-next at 308ee8a