linux-kernel - Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABxcv=kqigrfX3ruOUW=wJU1cAs1DmN41AmrUjYRB5f0phhy7g@mail.gmail.com>
Date:	Wed, 16 Sep 2015 00:26:09 +0200
From:	Javier Martinez Canillas <javier@...hile0.org>
To:	Emilio López <emilio.lopez@...labora.co.uk>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Olof Johansson <olof@...om.net>, Kukjin Kim <kgene@...nel.org>,
	Krzysztof Kozłowski <k.kozlowski@...sung.com>,
	Guenter Roeck <linux@...ck-us.net>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context

Hello Emilio,

On Tue, Sep 15, 2015 at 10:24 PM, Emilio López
<emilio.lopez@...labora.co.uk> wrote:

[snip]

>>>>> +
>>>>> +       params = (struct ec_params_vbnvcontext *)msg->data;
>>>>> +       params->op = EC_VBNV_CONTEXT_OP_READ;
>>>>> +
>>>>> +       msg->version = EC_VER_VBNV_CONTEXT;
>>>>> +       msg->command = EC_CMD_VBNV_CONTEXT;
>>>>> +       msg->outsize = sizeof(params->op);
>>>>
>>>>
>>>>
>>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>>> struct ec_params_vbnvcontext and not only the op field.
>>>>
>>>> Or if the EC only expects to get the u32 op field, then I think your
>>>> max payload calculation is not correct.
>>>
>>>
>>>
>>> The params struct is the same for both read and write ops, so it has the
>>> op
>>
>>
>> That's not true, struct ec_response_vbnvcontext has only the block
>> field while struct ec_param_vbnvcontext has both the op and block
>> fields.
>
>
> The former is a response struct, not a params struct.
>

I misread read/write as request/response in the previous email, sorry
about that.

>>> flag and a buffer for the write op. During the read op I believe there's
>>> no
>>> need to send this potentially-garbage-filled buffer to the EC, so outsize
>>> is
>>> set accordingly here.
>>
>>
>> Yes, I agree with you but then as I mentioned I think your payload
>> calculation is wrong since you want instead max(sizeof(struct
>> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
>> allocating 4 bytes more than needed.
>
>
> Yeah, I can see that. If I do that though, max(...) would be less than the
> size of the full params struct, and casting data to it could lead to subtle
> bugs in the future. I can change it and add a comment mentioning this, deal?
>

But by setting outsize to sizeof(params->op) you are allocating less
than the params struct anyways in the transport driver. Take a look
for example to cros_ec_cmd_xfer_i2c():

http://lxr.free-electrons.com/source/drivers/mfd/cros_ec_i2c.c#L187

But I don't have a strong opinion on this tbh, I was just pointing out
that it's strange that max(insize,outsize) does not match
msg->{insize,outsize}.

> (...)
>
>> with the needed changes, feel free to add my:
>>
>> Reviewed-by: Javier Martinez Canillas <javier@....samsung.com>
>
>
> Ok, thanks!
>
> Emilio

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/