Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
kernel/bpf/verifier.c
between commits:
b5871dca250c ("bpf: Simplify alu_limit masking for pointer arithmetic")
1b1597e64e1a ("bpf: Add sanity check for upper ptr_limit")
from the net tree and commit:
69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper")
from the net-next tree.
I fixed it up (see below - but it may need more work on the new
"return" starement from the latter commit) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc kernel/bpf/verifier.c
index 44e4ec1640f1,f9096b049cd6..000000000000
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@@ -5876,10 -6056,22 +6060,23 @@@ static int retrieve_ptr_limit(const str
if (mask_to_left)
*ptr_limit = MAX_BPF_STACK + off;
else
- *ptr_limit = -off;
- return 0;
+ *ptr_limit = -off - 1;
+ return *ptr_limit >= max ? -ERANGE : 0;
+ case PTR_TO_MAP_KEY:
+ /* Currently, this code is not exercised as the only use
+ * is bpf_for_each_map_elem() helper which requires
+ * bpf_capble. The code has been tested manually for
+ * future use.
+ */
+ if (mask_to_left) {
+ *ptr_limit = ptr_reg->umax_value + ptr_reg->off;
+ } else {
+ off = ptr_reg->smin_value + ptr_reg->off;
+ *ptr_limit = ptr_reg->map_ptr->key_size - off;
+ }
+ return 0;
case PTR_TO_MAP_VALUE:
+ max = ptr_reg->map_ptr->value_size;
if (mask_to_left) {
*ptr_limit = ptr_reg->umax_value + ptr_reg->off;
} else {
On 3/19/21 3:11 AM, Piotr Krysiuk wrote:
> Hi Daniel,
>
> On Fri, Mar 19, 2021 at 12:16 AM Stephen Rothwell <[email protected]>
> wrote:
>
>> diff --cc kernel/bpf/verifier.c
>> index 44e4ec1640f1,f9096b049cd6..000000000000
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@@ -5876,10 -6056,22 +6060,23 @@@ static int retrieve_ptr_limit(const str
>> if (mask_to_left)
>> *ptr_limit = MAX_BPF_STACK + off;
>> else
>> - *ptr_limit = -off;
>> - return 0;
>> + *ptr_limit = -off - 1;
>> + return *ptr_limit >= max ? -ERANGE : 0;
>> + case PTR_TO_MAP_KEY:
>> + /* Currently, this code is not exercised as the only use
>> + * is bpf_for_each_map_elem() helper which requires
>> + * bpf_capble. The code has been tested manually for
>> + * future use.
>> + */
>> + if (mask_to_left) {
>> + *ptr_limit = ptr_reg->umax_value + ptr_reg->off;
>> + } else {
>> + off = ptr_reg->smin_value + ptr_reg->off;
>> + *ptr_limit = ptr_reg->map_ptr->key_size - off;
>> + }
>> + return 0;
>>
>
> PTR_TO_MAP_VALUE logic above looks like copy-paste of old PTR_TO_MAP_VALUE
> code from before "bpf: Fix off-by-one for area size in creating mask to
> left" and is apparently affected by the same off-by-one, except this time
> on "key_size" area and not "value_size".
>
> This needs to be fixed in the same way as we did with PTR_TO_MAP_VALUE.
> What is the best way to proceed?
Hm, not sure why PTR_TO_MAP_KEY was added by 69c087ba6225 in the first place, I
presume noone expects this to be used from unprivileged as the comment says.
Resolution should be to remove the PTR_TO_MAP_KEY case entirely from that switch
until we have an actual user.
Thanks,
Daniel
On 3/19/21 12:21 AM, Daniel Borkmann wrote:
> On 3/19/21 3:11 AM, Piotr Krysiuk wrote:
>> Hi Daniel,
>>
>> On Fri, Mar 19, 2021 at 12:16 AM Stephen Rothwell <[email protected]>
>> wrote:
>>
>>> diff --cc kernel/bpf/verifier.c
>>> index 44e4ec1640f1,f9096b049cd6..000000000000
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@@ -5876,10 -6056,22 +6060,23 @@@ static int
>>> retrieve_ptr_limit(const str
>>> if (mask_to_left)
>>> *ptr_limit = MAX_BPF_STACK + off;
>>> else
>>> - *ptr_limit = -off;
>>> - return 0;
>>> + *ptr_limit = -off - 1;
>>> + return *ptr_limit >= max ? -ERANGE : 0;
>>> + case PTR_TO_MAP_KEY:
>>> + /* Currently, this code is not exercised as the only use
>>> + * is bpf_for_each_map_elem() helper which requires
>>> + * bpf_capble. The code has been tested manually for
>>> + * future use.
>>> + */
>>> + if (mask_to_left) {
>>> + *ptr_limit = ptr_reg->umax_value + ptr_reg->off;
>>> + } else {
>>> + off = ptr_reg->smin_value + ptr_reg->off;
>>> + *ptr_limit = ptr_reg->map_ptr->key_size - off;
>>> + }
>>> + return 0;
>>>
>>
>> PTR_TO_MAP_VALUE logic above looks like copy-paste of old
>> PTR_TO_MAP_VALUE
>> code from before "bpf: Fix off-by-one for area size in creating mask to
>> left" and is apparently affected by the same off-by-one, except this time
>> on "key_size" area and not "value_size".
>>
>> This needs to be fixed in the same way as we did with PTR_TO_MAP_VALUE.
>> What is the best way to proceed?
>
> Hm, not sure why PTR_TO_MAP_KEY was added by 69c087ba6225 in the first
> place, I
> presume noone expects this to be used from unprivileged as the comment
> says.
> Resolution should be to remove the PTR_TO_MAP_KEY case entirely from
> that switch
> until we have an actual user.
Alexei suggested so that we don't forget it in the future if
bpf_capable() requirement is removed.
https://lore.kernel.org/bpf/[email protected]/
I am okay with either way, fix it or remove it.
>
> Thanks,
> Daniel
On Fri, Mar 19, 2021 at 8:17 AM Yonghong Song <[email protected]> wrote:
>
>
>
> On 3/19/21 12:21 AM, Daniel Borkmann wrote:
> > On 3/19/21 3:11 AM, Piotr Krysiuk wrote:
> >> Hi Daniel,
> >>
> >> On Fri, Mar 19, 2021 at 12:16 AM Stephen Rothwell <[email protected]>
> >> wrote:
> >>
> >>> diff --cc kernel/bpf/verifier.c
> >>> index 44e4ec1640f1,f9096b049cd6..000000000000
> >>> --- a/kernel/bpf/verifier.c
> >>> +++ b/kernel/bpf/verifier.c
> >>> @@@ -5876,10 -6056,22 +6060,23 @@@ static int
> >>> retrieve_ptr_limit(const str
> >>> if (mask_to_left)
> >>> *ptr_limit = MAX_BPF_STACK + off;
> >>> else
> >>> - *ptr_limit = -off;
> >>> - return 0;
> >>> + *ptr_limit = -off - 1;
> >>> + return *ptr_limit >= max ? -ERANGE : 0;
> >>> + case PTR_TO_MAP_KEY:
> >>> + /* Currently, this code is not exercised as the only use
> >>> + * is bpf_for_each_map_elem() helper which requires
> >>> + * bpf_capble. The code has been tested manually for
> >>> + * future use.
> >>> + */
> >>> + if (mask_to_left) {
> >>> + *ptr_limit = ptr_reg->umax_value + ptr_reg->off;
> >>> + } else {
> >>> + off = ptr_reg->smin_value + ptr_reg->off;
> >>> + *ptr_limit = ptr_reg->map_ptr->key_size - off;
> >>> + }
> >>> + return 0;
> >>>
> >>
> >> PTR_TO_MAP_VALUE logic above looks like copy-paste of old
> >> PTR_TO_MAP_VALUE
> >> code from before "bpf: Fix off-by-one for area size in creating mask to
> >> left" and is apparently affected by the same off-by-one, except this time
> >> on "key_size" area and not "value_size".
> >>
> >> This needs to be fixed in the same way as we did with PTR_TO_MAP_VALUE.
> >> What is the best way to proceed?
> >
> > Hm, not sure why PTR_TO_MAP_KEY was added by 69c087ba6225 in the first
> > place, I
> > presume noone expects this to be used from unprivileged as the comment
> > says.
> > Resolution should be to remove the PTR_TO_MAP_KEY case entirely from
> > that switch
> > until we have an actual user.
>
> Alexei suggested so that we don't forget it in the future if
> bpf_capable() requirement is removed.
> https://lore.kernel.org/bpf/[email protected]/
>
> I am okay with either way, fix it or remove it.
I prefer to fix it.
On 3/19/21 4:33 PM, Alexei Starovoitov wrote:
> On Fri, Mar 19, 2021 at 8:17 AM Yonghong Song <[email protected]> wrote:
>> On 3/19/21 12:21 AM, Daniel Borkmann wrote:
>>> On 3/19/21 3:11 AM, Piotr Krysiuk wrote:
>>>> Hi Daniel,
>>>>
>>>> On Fri, Mar 19, 2021 at 12:16 AM Stephen Rothwell <[email protected]>
>>>> wrote:
>>>>
>>>>> diff --cc kernel/bpf/verifier.c
>>>>> index 44e4ec1640f1,f9096b049cd6..000000000000
>>>>> --- a/kernel/bpf/verifier.c
>>>>> +++ b/kernel/bpf/verifier.c
>>>>> @@@ -5876,10 -6056,22 +6060,23 @@@ static int
>>>>> retrieve_ptr_limit(const str
>>>>> if (mask_to_left)
>>>>> *ptr_limit = MAX_BPF_STACK + off;
>>>>> else
>>>>> - *ptr_limit = -off;
>>>>> - return 0;
>>>>> + *ptr_limit = -off - 1;
>>>>> + return *ptr_limit >= max ? -ERANGE : 0;
>>>>> + case PTR_TO_MAP_KEY:
>>>>> + /* Currently, this code is not exercised as the only use
>>>>> + * is bpf_for_each_map_elem() helper which requires
>>>>> + * bpf_capble. The code has been tested manually for
>>>>> + * future use.
>>>>> + */
>>>>> + if (mask_to_left) {
>>>>> + *ptr_limit = ptr_reg->umax_value + ptr_reg->off;
>>>>> + } else {
>>>>> + off = ptr_reg->smin_value + ptr_reg->off;
>>>>> + *ptr_limit = ptr_reg->map_ptr->key_size - off;
>>>>> + }
>>>>> + return 0;
>>>>
>>>> PTR_TO_MAP_VALUE logic above looks like copy-paste of old
>>>> PTR_TO_MAP_VALUE
>>>> code from before "bpf: Fix off-by-one for area size in creating mask to
>>>> left" and is apparently affected by the same off-by-one, except this time
>>>> on "key_size" area and not "value_size".
>>>>
>>>> This needs to be fixed in the same way as we did with PTR_TO_MAP_VALUE.
>>>> What is the best way to proceed?
>>>
>>> Hm, not sure why PTR_TO_MAP_KEY was added by 69c087ba6225 in the first
>>> place, I
>>> presume noone expects this to be used from unprivileged as the comment
>>> says.
>>> Resolution should be to remove the PTR_TO_MAP_KEY case entirely from
>>> that switch
>>> until we have an actual user.
>>
>> Alexei suggested so that we don't forget it in the future if
>> bpf_capable() requirement is removed.
>> https://lore.kernel.org/bpf/[email protected]/
>>
>> I am okay with either way, fix it or remove it.
>
> I prefer to fix it.
If the bpf_capable() is removed, the verifier would bail out on PTR_TO_MAP_KEY
if not covered in the switch given the recent fixes we did. I can fix it up after
merge if we think bpf_for_each_map_elem() will be used by unpriv in future..